Thanks for the review and sorry for the delay. The updated patch addresses
nits.
https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc
File src/ast-numbering.cc (right):
https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#newcode48
src/ast-numbering.cc:48: properties_.flags()->Add(kDontSelfOptimize);
On 2014/10/29 09:12:55, Sven Panne wrote:
Can we delegate between all these helpers and rely on inlining for
performance?
This would make the implications much clearer. In this case:
dont_crankshaft_reason_ = reason;
DisableSelfOptimization();
Done.
https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#newcode59
src/ast-numbering.cc:59: properties_.add_node_count(1);
On 2014/10/29 09:12:55, Sven Panne wrote:
Huh? Why do we increment again here (and in DisableCaching)? This
looks wrong...
Yes, a bug from a previous version of the patch. Good catch! I re-ran
MandreelLatency with this patch applied on master and fixing this
problem didn't have any perf impact, which I guess makes sense as the
node count is used for inlining and these cases wouldn't be optimized
anyway.
https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc#newcode581
src/compiler.cc:581: static void
MaybeDisableOptimization(Handle<SharedFunctionInfo> shared_info,
On 2014/10/29 09:12:55, Sven Panne wrote:
I think this should be folded into DisableOptimization, which should
probably be
renamed then. Hmmm, SetBailoutReason makes sense, but we already have
set_bailout_reason in that class (with a getter
DisableOptimizationReason,
*very* consistent naming :-P). OK, so my proposal would be:
* Rename DisableOptimization to SetBailoutReason and include the
kNoReason
handling there.
* Rename set_bailout_reason to set_disable_optimization_reason and
DisableOptimizationReason to disable_optimization_reason.
So, I did the renames to {set_,}disable_optimization_reason then looked
at DisableOptimization. Thing is, DisableOptimization does not just
update flags in the shared function info -- it also updates flags on the
code object, and the code object can change. I think
MaybeDisableOptimization is the probably the right solution here. For
sanity I added an assert to DisableOptimization that the reason was not
kNoReason. WDYT?
https://codereview.chromium.org/668143003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
