Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Optimize UnicodeICU localeCompare (#1186)
Summary: This change eliminates repeated calls to `ucol_open` and `ucol_close` in `hermes::platform_unicode::localeCompare` for HERMES_PLATFORM_UNICODE_ICU. On a sample sort.js(that sorted 30k strings using localeCompare), the runtime goes down from ~500 ms to ~120ms. **Before:** I observed in my trace that ucol_open and ucol_close was called in every localeCompare invocation and each ucol_open took around 500-900 ns. ucol_close is tiny ~100ish ns in each call. Note that the first call to ucol_open, it takes around 100us even after applying this diff. ![image](https://github.com/facebook/hermes/assets/6753926/739edffb-6c7b-4e93-babd-f51f9add4231) Here is localeCompare call stack before the patch: ![image](https://github.com/facebook/hermes/assets/6753926/9b9fbf31-aa5f-4cc6-a594-26c544c6ebcb) **After** After making UCollator construction static, we avoid repeated ucol_open and ucol_close(except during static initialization) thus making the sort.js sorting fast. Here is the localeCompare stack after the patch: ![image](https://github.com/facebook/hermes/assets/6753926/7deeba56-ec64-480d-9cf2-4ddcec10fc8b) Looking at the code, i found out that HERMES_PLATFORM_UNICODE_ICU is not used in Android/iOS but can make desktop usage faster (it it useful??). Here is the sort.js script which i got from one of the issues/posts in hermes. ``` print("gen..."); const randInt = (end, start = 0) => Math.floor(Math.random() * (end - start) + start); const alphabet = 'abcdefghijklmnopqrstuvwxyz'; const names = Array.from({length: 30000}, () => Array.from({length: 8}, () => alphabet[randInt(alphabet.length)]).join(''), ); for(var i = 0; i < 5; ++i){ print(names[i]) } print('sorting...'); const s = Date.now(); names.sort(localeCompare); print(`...done, took ${Date.now() - s}ms`); function localeCompare(a, b) { const s = Date.now(); const result = a.localeCompare(b); const e = Date.now(); if (e - s > 1) { print(`slow localeCompare: ${e - s}ms for '${a}' vs '${b}'`); } return result; } for(var i = 0; i < 5; ++i){ print(names[i]) } ``` Pull Request resolved: #1186 Test Plan: **Execute sort.js before applying this patch** ``` hermes (main)$ ../build_release/bin/hermesc -emit-binary -out sort.hbc samples/sort.js hermes (main)$ ../build_release/bin/hvm ./sort.hbc gen... sorting... ...done, took 466ms hermes (main)$ ../build_release/bin/hvm ./sort.hbc gen... sorting... ...done, took 486ms ``` **Execute sort.js after applying this patch** ``` $ ../build_release/bin/hermesc -emit-binary -out sort.hbc samples/sort.js hermes (icu_localecompare_optimize)$ ../build_release/bin/hvm ./sort.hbc gen... sorting... ...done, took 119ms hermes (icu_localecompare_optimize)$ ../build_release/bin/hvm ./sort.hbc gen... sorting... ...done, took 127ms ``` **Run check-hermes:** ``` hermes (icu_localecompare_optimize)$ cmake --build ../build_release/ --target check-hermes [0/1] Running the Hermes regression tests Testing Time: 15.91s Expected Passes : 1773 Unsupported Tests : 66 hermes (icu_localecompare_optimize)$ echo $? 0 ``` Reviewed By: avp Differential Revision: D51429400 Pulled By: neildhar fbshipit-source-id: f5a2f56952ec9bcbaad441df8f2e9c49c8a0b09a
- Loading branch information