-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize and fix autolink function (#1442) #1444
Conversation
Can you try to have this bug covered by an automated testcase ? |
I'd love to, but there are no automated tests for the front-end AFAIK. Would you like me to build a unit testing environment just to test this case? |
I would *love* you to build a unit testing environment,
not just to test this case :)
|
Since we don't have a test setup for the font-end yet, this should not be a block for this PR. That should be discussed in a proposal first, and than made in another PR. |
I just asked for the test because a fix and a speed improvements
are reported but wouldn't be easy to perform the test required
to prove it. As we need 2 reviews, it'll take 2 people to perform
independent tests. Having an automated test would just take verifying
the test once :)
|
Reviewing the fix is quite easy. For your convenience I made a benchmark for the performance difference: For those who can't be bothered to try it out, my optimized implementation will be about 30-50% faster on average. |
Nice app, codepen.io, first time I see it !
On Firefox version 45.3.0 on a 64bit GNU/Linux
this is what I get for 1000 loops:
JQuery: 451.3350000000082 ms
TreeWalker: 474.62999999999556 ms
Does it mean your change would make it slower there ?
I guess Chrome based browsers would have much different numbers
|
Same system, 10000 iterations:
JQuery: 3595.914999999368 ms
TreeWalker: 2506.635000000824 ms
LGTM :)
|
Yeah, it can be quirky at times. I added a warm-up before the tests, but the results are still not super consistent. I guess it's because there are a ton of factors involved. On average though it will perform significantly better. |
LGTM |
This PR fixes the markdown rendering issues with code-blocks containing URLs (#1442). The culprit was the
autolink
function, which conflicted with highlight.js. I've excluded code-block URLs from being auto-linked to fix this issue.Besides being broken the
autolink
function was also crazy inefficient. I've improved the efficiency by making use of the browser-nativecreateTreeWalker
function. If this function is not available (IE8 and below) I make use of a recursive function (which is still more efficient than the previous implementation).