RESOLVED FIXED 119961
REGRESSION(r154218): DFG::FixupPhase no longer turns GetById's child1 into CellUse 
https://bugs.webkit.org/show_bug.cgi?id=119961
Summary REGRESSION(r154218): DFG::FixupPhase no longer turns GetById's child1 into Ce...
Filip Pizlo
Reported 2013-08-17 14:43:45 PDT
This isn't a huge deal but we should fix it. Patch forthcoming.
Attachments
the patch (1.10 KB, patch)
2013-08-17 14:44 PDT, Filip Pizlo 		mhahnenberg: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2013-08-17 14:44:36 PDT
Created attachment 209010 [details] the patch
Mark Hahnenberg
Comment 2 2013-08-17 14:45:08 PDT
Comment on attachment 209010 [details] the patch r=me
Filip Pizlo
Comment 3 2013-08-17 22:45:34 PDT
Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next.
Oliver Hunt
Comment 4 2013-08-18 10:05:11 PDT
(In reply to comment #3) > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. Calling number prototype methods perhaps?
Filip Pizlo
Comment 5 2013-08-18 10:12:15 PDT
(In reply to comment #3) > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. Yup, that was a speed-up, and I missed it. OK, so we have some nastiness in our profiling, apparently. It shouldn't be expensive to use more speculation if we have logic to use *less* speculation instead.
Filip Pizlo
Comment 6 2013-08-18 10:14:21 PDT
(In reply to comment #4) > (In reply to comment #3) > > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. > > Calling number prototype methods perhaps? That would only explain it if we didn't also have the feature to disable cell speculation if profiling said that the input might not be a cell.
Filip Pizlo
Comment 7 2013-08-18 20:37:10 PDT
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. > > > > Calling number prototype methods perhaps? > > That would only explain it if we didn't also have the feature to disable cell speculation if profiling said that the input might not be a cell. OK, so this was the mother of all wild goose chases. It was literally the case that on the computer I was using, you got a 3% overall speed-up on DSP if you were built in a directory called "/Volumes/Data/pizlo/OpenSource". Lol.
Filip Pizlo
Comment 8 2013-08-18 20:39:10 PDT
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. > > > > > > Calling number prototype methods perhaps? > > > > That would only explain it if we didn't also have the feature to disable cell speculation if profiling said that the input might not be a cell. > > OK, so this was the mother of all wild goose chases. > > It was literally the case that on the computer I was using, you got a 3% overall speed-up on DSP if you were built in a directory called "/Volumes/Data/pizlo/OpenSource". > > Lol. I'm still retesting this ... sadly it'll take a bit of time since I have to build all of WebKit to run those benchmarks.
Filip Pizlo
Comment 9 2013-08-18 21:44:57 PDT
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #4) > > > > (In reply to comment #3) > > > > > Strangely enough, this leads to a slow-down on DSP tests. That implies that (1) those tests are heavy users of GetById and (2) they want to be able to do GetById on things that aren't cells. Weird. I'm investigating more. I don't remember seeing a progression on those tests in http://trac.webkit.org/changeset/154218; I'll test that next. > > > > > > > > Calling number prototype methods perhaps? > > > > > > That would only explain it if we didn't also have the feature to disable cell speculation if profiling said that the input might not be a cell. > > > > OK, so this was the mother of all wild goose chases. > > > > It was literally the case that on the computer I was using, you got a 3% overall speed-up on DSP if you were built in a directory called "/Volumes/Data/pizlo/OpenSource". > > > > Lol. > > I'm still retesting this ... sadly it'll take a bit of time since I have to build all of WebKit to run those benchmarks. Yeah. Total fluke.
Filip Pizlo
Comment 10 2013-08-18 21:46:07 PDT
Landed in http://trac.webkit.org/changeset/154261
Note You need to log in before you can comment on or make changes to this bug.