| Summary: | JITOperations getByVal should mark negative array indices as out-of-bounds | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||
| Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Tadeu Zagallo
2019-05-24 13:39:37 PDT
Created attachment 370586 [details]
Patch
Comment on attachment 370586 [details]
Patch
Nice. r=me
Comment on attachment 370586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370586&action=review > Source/JavaScriptCore/ChangeLog:10 > + but baseline doesn't mark it as out-of-bounds, since it only considers positive indices. This > + leads to discarding DFG code, recompiling it and exiting at the same bytecode. Can you quote where in JS2 you found this w/ the expected speedup? Created attachment 370630 [details]
Patch for landing
I think I might have messed up the measurements, let me double-check it before landing. Comment on attachment 370630 [details] Patch for landing Clearing flags on attachment: 370630 Committed r245769: <https://trac.webkit.org/changeset/245769> All reviewed patches have been landed. Closing bug. (In reply to Tadeu Zagallo from comment #5) > I think I might have messed up the measurements, let me double-check it > before landing. I was interested in how much faster the individual test gets. IIRC, on IRC, you said something like 5%-10%. I wouldn’t expect 5% on an individual test to show up as a measurable overall progression. When I make changes like this, I like to quote the impact on both the individual JS2 test and on the microbenchmark (In reply to Saam Barati from comment #9) > (In reply to Tadeu Zagallo from comment #5) > > I think I might have messed up the measurements, let me double-check it > > before landing. > > I was interested in how much faster the individual test gets. IIRC, on IRC, > you said something like 5%-10%. I wouldn’t expect 5% on an individual test > to show up as a measurable overall progression. When I make changes like > this, I like to quote the impact on both the individual JS2 test and on the > microbenchmark Oops, my bad, will keep that in mind going forward. FWIW, it looks like a ~10% progression in prepack-wtb (which I'm a bit skeptical of, since the function wasn't that hot) and the microbenchmark is ~4% faster, but is within the noise (maybe it wasn't a great microbenchmark after all...) Nice! Seems that on iOS devices it gets 1-3% improvement in JetStream2/uglify-js-wtb |