- Fix for Issue #391 #1509
- Fix for Issue #391 #1509
Conversation
- Fix for Issue #391
ghost
assigned 
|
Nice Fix! Thank you also for the unit tests! Before we can merge this into master, it must pass JSLint. Turn it on in Brackets using: View > JSLint . |
|
|
||
| // Is faster regexp.test() than a replace with no hits? | ||
| //if(/\\/.test(currentSelector)) | ||
| //{ |
There was a problem hiding this comment.
There is a basic performance testing timer builtin to Brackets called PerfUtils that you can use to time both cases. Use it something like this:
PerfUtils.markStart("[descriptive string]");
// code to measure ...
PerfUtils.addMeasurement("[descriptive string]"); // same string passs to markStart()
Then use Debug > Show Performance Data to see min/max/avg times measured.
I measured this both ways and saw a max timing of 1ms, so this code does not seem have a significant impact. Here's a way you can try to get sub ms timing info (but I haven't tried it, so not sure if it works):
http://updates.html5rocks.com/2012/08/When-milliseconds-are-not-enough-performance-now
I would think that regexp.test() would be faster than a replace with no hits, so please uncomment these lines.
There was a problem hiding this comment.
I did a test run with jsperf and it seems that regexp.test() is around 30% faster.
There was a problem hiding this comment.
Excellent usage of jsperf! So, you're randomly generating selectors from a random length of alpha-numeric characters, right? I don't think that's a representative set of data for your code since there are no escaped chars. Maybe you could build the array of selectors with a hard-coded list of mostly normal selectors, then mix in the ones with escaped chars from your unit tests.
There was a problem hiding this comment.
The goal of the previous test was just to find out if a simple regex test was faster than a replace when the pattern in the replace yields no hits. It was focused on the subset of data where no unicode chars are found in the selectors. That's why I didn't add any escaped chars.
In any case, and to be more thorough, I've created a second test. In this one, some characters are being randomly substituted with their escaped equivalents. This ensures that the replace code is executed. As expected, the version with the if(regex.test()) is slightly slower as it's executing the same piece of code plus the additional if operator.
To complete a study on this, I think we'd need some usage statistics of escaped characters in css so that we could model how often this happens (I guess not that often anyway :D). With that data, we could compare the overall impact of this fix and the difference in performance between using or not the first if clause. In any case, I assume using escaped characters in css is pretty rare so I'm confident in using the test() solution based on the results of the first test.
|
Done with initial code review. |
- Added conditional regex.test() since it is faster than just replace
| //var escapePattern = /\\[^\\]+/g; | ||
| var escapePattern = new RegExp('\\\\[^\\\\]+', "g"); | ||
| //var validationPattern = /\\([a-f0-9]{6}|[a-f0-9]{4}(\s|\\|$)|[a-f0-9]{2}(\s|\\|$)|.)/i; | ||
| var validationPattern = new RegExp('\\\\([a-f0-9]{6}|[a-f0-9]{4}(\\s|\\\\|$)|[a-f0-9]{2}(\\s|\\\\|$)|.)', "i"); |
There was a problem hiding this comment.
Please remove the commented out lines.
|
Looks great. Just a couple code cleanup comments. |
|
Looks good. Merging. |
Possible fix for Issue #391.
I've created a css test file with all the possible cases I could think of plus the ones from the less suite test.
CSSUtils-test.jshas also been modified with the unit tests expecting the parsed selectors fromCSSUtils.extractAllSelectors()to match the decoded string.The fix is based on a double replace using two regular expressions. It's left to check if a simple regexp test to avoid the replace call is faster than the replace call itself when its own regexp produces no matches (I guess it should be). Also maybe someone can improve the regular expressions and combine them to avoid the second replace call?