Closed Bug 1578339 Opened 5 years ago Closed 5 years ago

Use SIMD-accelerated Latin1ness checking, inflation and deflation

Categories

(Core :: JavaScript Engine, enhancement, P2)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1578339 - Use SIMD accelerated encoding conversions in SpiderMonkey.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1578339 addendum - Avoid function calls in ASCIIness and Latin1ness checking and conversion between Latin1 and UTF-16 for short strings.
47 bytes, text/x-phabricator-request
Details | Review

Bug 1490601 makes SIMD-accelerated (on tier1 architectures) operations mozilla::IsUtf16Latin1, mozilla::LossyConvertUtf16toLatin1, and mozilla::ConvertLatin1toUtf16 available to SpiderMonkey. Let's use them in SpiderMonkey.

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Priority: -- → P2

There are a bunch of UTF-8 conversions that I didn't change in this patch. I'll file follow-ups for those.

Attachment #9090056 - Attachment is obsolete: true

Thinking about it more, I think we should block this on bug 1578677. We don't want the shell and browser to diverge more than necessary - security (fuzzing) depends on it.

Depends on: 1578677

Can you point me to an existing example of the type of microbenchmark that I should write to show that this improves things?

Flags: needinfo?(jdemooij)

(In reply to Henri Sivonen (:hsivonen) from comment #8)

Can you point me to an existing example of the type of microbenchmark that I should write to show that this improves things?

Sorry for the delay. Looking at the callers, this should end up in CanStoreCharsAsLatin1, probably worth testing also with just a few arguments:

function f() {
    var t = new Date;
    for (var i = 0; i < 1000000; i++) {
        s = String.fromCodePoint(50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
                                 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60);
    }
    print(new Date - t);
}
f();
Flags: needinfo?(jdemooij)

Thanks. So far (I need to test a bit more), I see a perf improvement for the case you gave but a slight regression if I shorten it to 7 code points. I think I should restore the function call avoidance bits that I deleted as unnecessary after we got cross-language LTO, since it appears that for SpiderMonkey, even the one level of function calls for short strings matters.
https://hg.mozilla.org/mozilla-central/rev/a3f2cf3e1bb4#l17.69

(In reply to Jan de Mooij [:jandem] from comment #7)

Thinking about it more, I think we should block this on bug 1578677. We don't want the shell and browser to diverge more than necessary - security (fuzzing) depends on it.

I filed bug 1583730 about cross-language LTO. Should it, too, be treated as a blocker for this (may affect benchmarking but should not affect fuzzing validity)?

(In reply to Henri Sivonen (:hsivonen) from comment #11)

I filed bug 1583730 about cross-language LTO. Should it, too, be treated as a blocker for this (may affect benchmarking but should not affect fuzzing validity)?

Would be nice to have (also for things like Cranelift) but probably doesn't have to block...

Benchmarking build with Latin1ness checking for short (len < 12) UTF-16 strings inlined:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f1c952c00c957ebdc80fb7e3677df32ff9015d2

Benchmarking build with Latin1 <-> UTF-16 conversions inlined for short (len < 16) strings:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b84f5fea045db2bcc271fcad97654d122278b75

Benchmarking builds with both checks as len < 16 to give the optimizer an opportunity to merge the checks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dce7b73b21152468992e561824459cc463bcd54

Attachment #9095158 - Attachment description: Bug 1578339 addendum - Restore function call avoidance for ASCIIness and Latin1ness checking of short UTF-16 strings. → Bug 1578339 addendum - Avoid function calls in ASCIIness and Latin1ness checking and conversion between Latin1 and UTF-16 for short strings.

(In reply to Henri Sivonen (:hsivonen) from comment #16)

Benchmarking builds with both checks as len < 16 to give the optimizer an opportunity to merge the checks:

Giving the optimizer this opportunity seemed to matter!

Blocks: 1561576
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/314012b65b23
Use SIMD accelerated encoding conversions in SpiderMonkey. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/cfe458fd3f84
addendum - Avoid function calls in ASCIIness and Latin1ness checking and conversion between Latin1 and UTF-16 for short strings. r=jandem

https://hg.mozilla.org/mozilla-central/rev/314012b65b23
https://hg.mozilla.org/mozilla-central/rev/cfe458fd3f84

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: