Bug 198229

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Tadeu Zagallo

Reported 2019-05-24 13:39:37 PDT

...

Attachments
Patch (4.11 KB, patch) 2019-05-24 13:59 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch for landing (4.43 KB, patch) 2019-05-25 00:55 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Tadeu Zagallo

Comment 1 2019-05-24 13:59:44 PDT

Created attachment 370586 [details] Patch

Saam Barati

Comment 2 2019-05-24 15:35:51 PDT

Comment on attachment 370586 [details] Patch Nice. r=me

Saam Barati

Comment 3 2019-05-24 15:36:22 PDT

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?

Tadeu Zagallo

Comment 4 2019-05-25 00:55:56 PDT

Created attachment 370630 [details] Patch for landing

Tadeu Zagallo

Comment 5 2019-05-25 01:01:05 PDT

I think I might have messed up the measurements, let me double-check it before landing.

WebKit Commit Bot

Comment 6 2019-05-25 02:25:31 PDT

Comment on attachment 370630 [details] Patch for landing Clearing flags on attachment: 370630 Committed r245769: <https://trac.webkit.org/changeset/245769>

WebKit Commit Bot

Comment 7 2019-05-25 02:25:33 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 8 2019-05-25 02:26:18 PDT

<rdar://problem/51134153>

Saam Barati

Comment 9 2019-05-25 07:50:47 PDT

(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

Tadeu Zagallo

Comment 10 2019-05-25 09:28:06 PDT

(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...)

Yusuke Suzuki

Comment 11 2019-05-25 20:47:49 PDT

Nice! Seems that on iOS devices it gets 1-3% improvement in JetStream2/uglify-js-wtb

Note You need to log in before you can comment on or make changes to this bug.