-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
improved speed of jade.escape() #1976
Conversation
, '"': '"' | ||
} | ||
, _MATCH_HTML = /[&<>"]/g; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants don't need the preceding _
Looks good. It will also need to be added to the jade-runtime module for 2.0.0 |
LGTM. Note that sometimes the performance of these escape functions can vary a lot from how you call them. For example, a while ago while tuning ejs I found out that this one you used fares a lot better on client mode (IIRC), while the original is better when you are just compiling and executing in the same environment. That's something to keep in mind when making performance changes. You should also check if aliasing the variables in the |
@TimothyGu I use both jsperf for the stuff that comes out of the client compilation as well as the server-side benchmark that I forked from yours - so far, I see much better performance in both scenarios and no performance regressions. Do you have a specific benchmark I should look at? |
@ForbesLindesay is the jade runtime already extracted into its own repo? The one in this org seems to be a little outdated. |
https://github.com/jadejs/jade-runtime is the 2.0.0 runtime. It is ahead of 1.0.0 runtime in a few key areas. If it's behind in anything then that's most likely a mistake. |
improved speed of jade.escape()
see #1975