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