Improve performance of the uts46 crate #453
Conversation
This might not be exact, but it should be a good estimate. I don't think this will ever over allocate in any case(?).
By making `MAPPING_TABLE` consist of slices we merge all runs of single character ranges into a single range and subtract the start of the range from the actual codepoint to figure out which `Mapping` each of those individual character has. This reduces the size of the table that is binary searched to about 1/4 which does not impact runtime performance very much but does remove about (6000 * 8 - 1500 * 16 =) 24kb from the resulting binary (with more improvements to follow)
`MAPPING_TABLE` were pretty large from the last commit as it contains two pointers for each element. By converting it into a index to another table + a single element indicator to recover most of it (6000 * 8 - 1500 * 4 =) 42kb reduction.
Since we don't need all bits we can store the first indicator in `u16` directly and save another 3kb
|
Looks good, thank you! Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 3 of 3 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, 1 of 1 files at r10. idna/src/make_uts46_mapping_table.py, line 130 at r5 (raw file):
Coding style nit: I find this hard to read when I need to keep elsewhere to find out what is at index 0, 1, 2. What do you think of using tuple deconstruction in the idna/src/make_uts46_mapping_table.py, line 164 at r7 (raw file):
If this boolean / bit still needs to be stored (see previous comment), I’d prefer the bitwise OR to be done in Python, since many more people will recompile the Rust code. Also add an assert that we don’t overflow 15 bits? idna/src/uts46.rs, line 75 at r6 (raw file):
Aren’t these two code paths the same? Maybe the boolean is not needed at all? Comments from Reviewable |
|
Review status: all files reviewed, 3 unresolved discussions (waiting on @Marwes) idna/src/make_uts46_mapping_table.py, line 130 at r5 (raw file): Previously, SimonSapin (Simon Sapin) wrote…
👍 idna/src/make_uts46_mapping_table.py, line 164 at r7 (raw file): Previously, SimonSapin (Simon Sapin) wrote…
Sure idna/src/uts46.rs, line 75 at r6 (raw file): Previously, SimonSapin (Simon Sapin) wrote…
It differentiates between the two cases
In the first case multiple codepoints needs to map to Comments from Reviewable |
|
Review status: all files reviewed, 2 unresolved discussions (waiting on @SimonSapin) idna/src/uts46.rs, line 75 at r6 (raw file): Previously, Marwes (Markus Westerlind) wrote…
Ok I see, thanks. Comments from Reviewable |
|
@SimonSapin That should take care of all comments. |
|
Great, thanks! @bors-servo r+ Reviewed 1 of 1 files at r11, 1 of 1 files at r12, 3 of 3 files at r13, 3 of 3 files at r14. Comments from Reviewable |
complete! all files reviewed, all discussions resolved
|
📌 Commit 5dfd1ec has been approved by |
Improve performance of the uts46 crate I may have complicated the table structure more than is desired so I'd be happy to remove it if you want. It does shrink the total size of the tables by 6000 * 8 - 1500 * 2 = 42kb though (and gives a slight performance boost). (Calculation is done as `removed_ranges * range_size - added_indexes * index_size`) Total performance change of parsing a small and simple url. ``` name old ns/iter new ns/iter diff ns/iter diff % speedup short 4,819 (5 MB/s) 3,516 (7 MB/s) -1,303 -27.04% x 1.37 ``` <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/453) <!-- Reviewable:end -->
|
☀️ Test successful - status-travis |
I may have complicated the table structure more than is desired so I'd be happy to remove it if you want. It does shrink the total size of the tables by 6000 * 8 - 1500 * 2 = 42kb though (and gives a slight performance boost).
(Calculation is done as
removed_ranges * range_size - added_indexes * index_size)Total performance change of parsing a small and simple url.
This change is