Opened 15 years ago
Closed 12 years ago
#11778 closed Cleanup/optimization (fixed)
Patch to increase performance of escapejs
Reported by: | josephdrose | Owned by: | josephdrose |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | jrose@…, Dmitry Jemerov | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The current implementation of escapejs iterates through a list of values and does a string.replace(...). The patch here creates a regular expression with the values and uses a dictionary to do the replace upon the match. Local testing shows the new implementation to be faster; the script used to perform this test is also attached.
Attachments (6)
Change History (17)
by , 15 years ago
Attachment: | test_escapejs.py added |
---|
by , 15 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
by , 15 years ago
Attachment: | escapejs.patch added |
---|
comment:2 by , 15 years ago
On my machine using josephdrose's benchmark script this approach doubles the speed of the escapejs() function.
A problem with the patch is that the set of chars to match now has to be maintained in 2 places: both where _base_js_escapes and _js_escapes_re are set. It would be an improvement to generate the re from _base_js_escapes.
by , 15 years ago
Attachment: | 0001-Faster-escapejs-11778.patch added |
---|
Even faster version without double maintenance of keys
comment:3 by , 15 years ago
Triage Stage: | Design decision needed → Unreviewed |
---|
I've updated the patch with a new version that is better because you don't have to maintain the keys in 2 places and is faster because all chars to be replaces is matched by a single regex-charclass. I've also attached a new benchmark script.
Unfortunately there are cases where the new approach actually is slower. This happens when we try to escape long strings with large fraction of chars that need to be escaped. It still think this patch is worth to apply because it should make this function 2-5 times faster on most "real" strings.
When I run the escapejs_benchmark.py script here I see:
11.5x speedup 2.1s vs 0.2s 'xxxxx'
5.4x speedup 3.9s vs 0.7s 'xxxxxxxxxx...'
2.9x speedup 2.2s vs 0.8s ""
5.0x slowdown 10.0s vs 49.6s "..."
2.4x speedup 2.5s vs 1.0s "Hello, I'm..."
comment:4 by , 15 years ago
With proper wikiformatting it looks like :-(
11.5x speedup 2.1s vs 0.2s 'xxxxx' 5.4x speedup 3.9s vs 0.7s 'xxxxxxxxxx...' 2.9x speedup 2.2s vs 0.8s "'''''" 5.0x slowdown 10.0s vs 49.6s "''''''''''..." 2.4x speedup 2.5s vs 1.0s "Hello, I'm..."
comment:5 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 15 years ago
milestone: | 1.2 |
---|
This isn't critical for the first 1.2 release. Bumping from the milestone.
comment:7 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
Severity: | → Normal |
Type: | → Uncategorized |
0001-Faster-escapejs-11778.patch fails to apply cleanly on to trunk
comment:8 by , 13 years ago
Type: | Uncategorized → Cleanup/optimization |
---|
comment:9 by , 13 years ago
Cc: | added |
---|---|
UI/UX: | unset |
Updated patch to latest trunk (r16344), updated expected test results due to the changes in escapejs output (now it uses \x## notation instead of \u#### for characters with low ASCII codes). The output change looks like a backward compatibility concern to me, although the new output is more compact than the old one (which saves a tiny bit of traffic).
by , 13 years ago
Attachment: | 11778.diff added |
---|
Patch against r16344 with updated expected test results
comment:10 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:11 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This has probably already been solved by [44767f2caf028d8].
Patch update to make it pass the 'defaultfilters' test (added match on \u2028\u2029)