On 2015/06/19 11:17:08, Michael Starzinger wrote:
LGTM on the implementation with a few comments. About the additional
memory
usage, I'll defer to Hannes and/or Toon.
https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/1183733006/diff/60001/src/compiler/ast-graph-builder.cc#newcode1526
src/compiler/ast-graph-builder.cc:1526: // We also have a stack overflow
if
the
recursive compilation did.
nit: Unrelated to your change, but can we just drop the second sentence
of the
comment? Because I still haven't built a test-case for this and am still
surprised that the DCHECK below never hits.
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6410
src/objects.h:6410: class SharedFunctionInfo;
nit: Can we just move this up to the forward declarations around line 880
at
the
top?
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6521
src/objects.h:6521: MaybeHandle<SharedFunctionInfo>
FindSharedFunctionInfo(FunctionLiteral* fun);
nit: IMHO this function needs short explaining comment.
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6523
src/objects.h:6523: static void AddSharedFunctionInfo(Handle<Script>
script,
nit: This shouldn't be needed anymore, correct?
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6675
src/objects.h:6675: static void SetScript(Handle<SharedFunctionInfo>
shared,
nit: Likewise, can we get a comment?
https://codereview.chromium.org/1183733006/diff/60001/src/objects.h#newcode6722
src/objects.h:6722: DECL_ACCESSORS(inner_functions, Object)
nit: This shouldn't be needed anymore, correct?
https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-function.cc
File src/runtime/runtime-function.cc (right):
https://codereview.chromium.org/1183733006/diff/60001/src/runtime/runtime-function.cc#newcode235
src/runtime/runtime-function.cc:235: SharedFunctionInfo::SetScript(
As discussed offline: It might make sense to visually separate this
handlified
call from the pure assignment by just moving it out of the block to around
line
246 for example. Applies here and to some other call-site as well.
Also: An interesting experiment might be to insert an explicit GC call
into
SetScript and see if that flushes out any bugs with SharedFunctionInfo
objects
being in an inconsistent state.
Addressed comments. I created a test CL
(https://codereview.chromium.org/1183733006/) to run the suggested GC
experiment.
https://codereview.chromium.org/1183733006/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
