Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174723002:
Move CallSite.toString to C++ (Closed)
DescriptionMove CallSite.toString to C++
BUG=
Committed: https://crrev.com/ea09c9dc1e2c44006930cdbcf00cb87dd350f487
Cr-Commit-Position: refs/heads/master@{#38136}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 7
Patch Set 5 : More efficient endsWith predicate #Patch Set 6 : Also accept method name == function name as a match #Patch Set 7 : Emulate JS behavior of boolean checks on strings #Patch Set 8 : Emulate JS behavior of boolean checks on strings #Patch Set 9 : Rebase #
Messages
Total messages: 43 (36 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9534) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9514) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9479) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5493) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20022)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9546) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jgruber@chromium.org changed reviewers: + yangguo@chromium.org
https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... File src/builtins/builtins-callsite.cc (right): https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:202: MaybeHandle<Object> GetEvalOrigin(Isolate* isolate, Handle<JSObject> object) { I'm inclined to move these helper functions to the CallSite class, similiar to CallSite::GetFileName. https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:332: MaybeHandle<Object> GetTypeName(Isolate* isolate, Handle<JSObject> object) { Same here. https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:508: isolate->factory()->NewConsString(isolate->factory()->dot_string(), This looks really inefficient to me. We want to know whether the function name ends with the method name, with a preceding ".". So can't we just call StringIndexOf with the function string and the method string, and check for preceding "."? Even better would be just to flatten and compare the string contents starting from end, using a FlatStringReader.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... File src/builtins/builtins-callsite.cc (right): https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:202: MaybeHandle<Object> GetEvalOrigin(Isolate* isolate, Handle<JSObject> object) { On 2016/07/28 06:04:10, Yang wrote: > I'm inclined to move these helper functions to the CallSite class, similiar to > CallSite::GetFileName. Yes that's a good point. I ran into the same thing while porting CallSite.toString, and I started moving helpers to the C++ CallSite class there. If you prefer, I could finish that work and backport it to this CL. https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:332: MaybeHandle<Object> GetTypeName(Isolate* isolate, Handle<JSObject> object) { On 2016/07/28 06:04:10, Yang wrote: > Same here. Acknowledged. https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:508: isolate->factory()->NewConsString(isolate->factory()->dot_string(), On 2016/07/28 06:04:10, Yang wrote: > This looks really inefficient to me. We want to know whether the function name > ends with the method name, with a preceding ".". So can't we just call > StringIndexOf with the function string and the method string, and check for > preceding "."? Even better would be just to flatten and compare the string > contents starting from end, using a FlatStringReader. Done. This also fixes a bug (already present in the original JS code) in which ends_with_method_name is set when function_string == method_string, since IndexOf returns -1 and fstr.len - mstr.len - 1 is also -1.
lgtm. https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... File src/builtins/builtins-callsite.cc (right): https://codereview.chromium.org/2174723002/diff/60001/src/builtins/builtins-c... src/builtins/builtins-callsite.cc:202: MaybeHandle<Object> GetEvalOrigin(Isolate* isolate, Handle<JSObject> object) { On 2016/07/28 09:21:50, jgruber wrote: > On 2016/07/28 06:04:10, Yang wrote: > > I'm inclined to move these helper functions to the CallSite class, similiar to > > CallSite::GetFileName. > > Yes that's a good point. I ran into the same thing while porting > CallSite.toString, and I started moving helpers to the C++ CallSite class there. > > If you prefer, I could finish that work and backport it to this CL. I'm fine with having that in a separate CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9882) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9864) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9826) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5852) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20454)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9884) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2174723002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Move CallSite.toString to C++ BUG= ========== to ========== Move CallSite.toString to C++ BUG= Committed: https://crrev.com/ea09c9dc1e2c44006930cdbcf00cb87dd350f487 Cr-Commit-Position: refs/heads/master@{#38136} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ea09c9dc1e2c44006930cdbcf00cb87dd350f487 Cr-Commit-Position: refs/heads/master@{#38136} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
