Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(160)

Issue 103243005: Captured arguments object materialization (Closed)

Created:
7 years ago by Jarin
Modified:
6 years, 10 months ago
CC:
v8-dev
Visibility:
Public.

Description

The current version is passing all the existing test + a bunch of new tests (packaged in the change list, too). The patch extends the SlotRef object to describe captured and duplicated objects. Since the SlotRefs are not independent of each other anymore, there is a new SlotRefValueBuilder class that stores the SlotRefs and later materializes the objects from the SlotRefs. Note that unlike the previous implementation of SlotRefs, we now build the SlotRef entries for the entire frame, not just the particular function. This is because duplicate objects might refer to previous captured objects (that might live inside other inlined function's part of the frame). We also need to store the materialized objects between other potential invocations of the same arguments object so that we materialize each captured object at most once. The materialized objects of frames live in the new MaterielizedObjectStore object (contained in Isolate), indexed by the frame's FP address. Each argument materialization (and deoptimization) tries to lookup its captured objects in the store before building new ones. Deoptimization also removes the materialized objects from the store. We also schedule a lazy deopt to be sure that we always get rid of the materialized objects and that the optmized function adopts the materialized objects (instead of happily computing with its captured representations). Concerns: - Is the FP address the right key for a frame? (Note that deoptimizer's representation of frame is different from the argument object materializer's one - it is not easy to find common ground.) - Performance is suboptimal in several places, but a quick local run of benchmarks does not seem to show a perf hit. Examples of possible improvements: smarter generation of SlotRefs (build other functions' SlotRefs only for captured objects and only if necessary), smarter lookup of stored materialized objects. - Ideally, we would like to share the code for argument materialization with deoptimizer's materializer. However, the supporting data structures (mainly the frame descriptor) are quite different in each case, so it looks more like a separate project. Thanks for any feedback. R=danno@chromium.org, mstarzinger@chromium.org LOG=N BUG= Committed: https://code.google.com/p/v8/source/detail?r=18918 Committed: https://code.google.com/p/v8/source/detail?r=18936

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Formatting fix #

Total comments: 10

Patch Set 4 : Addressed code review comments #

Total comments: 10

Patch Set 5 : Direct reference to materialized objects from GC root. #

Patch Set 6 : Clarify comments, fix formatting. #

Patch Set 7 : Uploading again... #

Patch Set 8 : Skip uninteresting frame types when building SlotRefs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+695 lines, -123 lines) Patch
M include/v8.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/accessors.cc View 1 2 1 chunk +9 lines, -8 lines 0 comments Download
M src/deoptimizer.h View 1 2 3 4 5 4 chunks +85 lines, -48 lines 0 comments Download
M src/deoptimizer.cc View 1 2 3 4 5 6 7 10 chunks +372 lines, -44 lines 0 comments Download
M src/heap.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 0 comments Download
M src/heap.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M src/isolate.h View 1 2 3 4 chunks +9 lines, -4 lines 0 comments Download
M src/isolate.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
M src/lithium.cc View 1 2 1 chunk +8 lines, -8 lines 0 comments Download
M src/runtime.cc View 1 2 3 1 chunk +8 lines, -9 lines 0 comments Download
A test/mjsunit/compiler/escape-analysis-arguments.js View 1 chunk +187 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Jarin
7 years ago (2013-12-23 22:22:51 UTC) #1
Michael Starzinger
Looking good. Just a couple of comments. About the concerns in your description I am ...
6 years, 11 months ago (2014-01-14 09:50:18 UTC) #2
Jarin
https://codereview.chromium.org/103243005/diff/230001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/103243005/diff/230001/src/deoptimizer.cc#newcode491 src/deoptimizer.cc:491: (*functions)[0]->context()->native_context()); On 2014/01/14 09:50:18, Michael Starzinger wrote: > This ...
6 years, 11 months ago (2014-01-27 15:40:57 UTC) #3
danno
https://codereview.chromium.org/103243005/diff/300001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/103243005/diff/300001/src/deoptimizer.cc#newcode3195 src/deoptimizer.cc:3195: prev_materialized_count_ = previously_materialized_objects_.is_null() ? nit: funky formatting. Put the ...
6 years, 10 months ago (2014-01-28 15:44:33 UTC) #4
Jarin
https://codereview.chromium.org/103243005/diff/300001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/103243005/diff/300001/src/deoptimizer.cc#newcode3195 src/deoptimizer.cc:3195: prev_materialized_count_ = previously_materialized_objects_.is_null() ? On 2014/01/28 15:44:34, danno wrote: ...
6 years, 10 months ago (2014-01-28 19:52:30 UTC) #5
Michael Starzinger
LGTM.
6 years, 10 months ago (2014-01-29 09:47:54 UTC) #6
Jarin
Committed patchset #7 manually as r18918 (presubmit successful).
6 years, 10 months ago (2014-01-29 15:14:30 UTC) #7
Jarin
On 2014/01/29 15:14:30, jarin wrote: > Committed patchset #7 manually as r18918 (presubmit successful). Had ...
6 years, 10 months ago (2014-01-30 09:31:06 UTC) #8
danno
lgtm
6 years, 10 months ago (2014-01-30 10:06:36 UTC) #9
Jarin
6 years, 10 months ago (2014-01-30 10:34:06 UTC) #10
Message was sent while issue was closed.
Committed patchset #8 manually as r18936 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698