Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962353002:
Add probe for V8.Complie (Closed)
|
Created:
3 years, 5 months ago by Liquan (Max) Gu Modified:
3 years, 4 months ago CC:
chromium-reviews, blink-reviews, hiroshige+script_chromium.org, kouhei+script_chromium.org, blink-reviews-bindings_chromium.org, kochi+script_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a probe for V8.Complie in order to measure the time of compiling script for V8.
BUG=738495
Review-Url: https://codereview.chromium.org/2962353002
Cr-Commit-Position: refs/heads/master@{#488471}
Committed: https://chromium.googlesource.com/chromium/src/+/e51107e73a6f93cfb447d6f5b432c100ef31d309
Patch Set 1 : add histogram #
Total comments: 2
Patch Set 2 : remove implementation #
Total comments: 4
Patch Set 3 : correct executionContext usage #
Total comments: 2
Patch Set 4 : Replace ScriptState with isolate and ExecutionState. #Patch Set 5 : fix tests #
Total comments: 4
Patch Set 6 : switch to use isolate--context--scriptState #Patch Set 7 : revert to "correct executionContext usage" #
Total comments: 2
Patch Set 8 : remove execution context, use nullptr #
Total comments: 1
Patch Set 9 : update Todos #
Messages
Total messages: 65 (26 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
maxlg@chromium.org changed reviewers: + panicker@chromium.org
Next step is to implement the measurement of time in the probe.
By the way, this issue haven't been buganized yet. Do we need to create one?
Description was changed from ========== v8Complie probe BUG= ========== to ========== Add a probe for V8.Complie in order to measure the time of compiling script for V8. BUG=738495 ==========
Do we care LongV8Compile especially or just V8Compile?
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
PTAL. Just to make sure it's going to the right direction.
panicker@chromium.org changed reviewers: + alph@chromium.org, caseq@chromium.org
looks sane overall. +caseq, alph
https://codereview.chromium.org/2962353002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right): https://codereview.chromium.org/2962353002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:207: probe.CaptureStartTime(); Let's move time capture to separate CL as it requires running benchmarks and verifying overhead is reasonable.
lgtm modulo Shubhie's comment.
https://codereview.chromium.org/2962353002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right): https://codereview.chromium.org/2962353002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:207: probe.CaptureStartTime(); On 2017/07/14 23:39:19, panicker wrote: > Let's move time capture to separate CL as it requires running benchmarks and > verifying overhead is reasonable. Done.
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2962353002/#ps120001 (title: "remove implementation")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
maxlg@chromium.org changed reviewers: + japhet@chromium.org
+japhet@ owner. PTAL, thanks!
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp
third_party/WebKit/Source/core/frame/PerformanceMonitor.h
haraken@chromium.org changed reviewers: + haraken@chromium.org
Would you help me understand why probe::V8Compile needs ExecutionContext? I guess it would make more sense to let it take v8::Context rather than ExecutionContext*. https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:131: ExecutionContext::From(ToScriptStateForMainWorld(GetFrame())), This won't give you a correct context. You'll need to call ExecutionContext::From(ScriptState::From(context)). https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: ExecutionContext::From(ScriptState::Current(isolate)), source, Alternately can you pass in ExecutionContext* to V8ScriptRunner::CompileAndRunInternalScript?
Thanks @haraken. I am not sure why we use execution context or v8::context actually, but there is no v8::context for ToCoreProbeSink in coreProbes.h, do we want to add it? https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:131: ExecutionContext::From(ToScriptStateForMainWorld(GetFrame())), On 2017/07/17 16:16:24, haraken wrote: > > This won't give you a correct context. > > You'll need to call ExecutionContext::From(ScriptState::From(context)). Thanks! https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: ExecutionContext::From(ScriptState::Current(isolate)), source, On 2017/07/17 16:16:24, haraken wrote: > > Alternately can you pass in ExecutionContext* to > V8ScriptRunner::CompileAndRunInternalScript? Done.
On 2017/07/17 20:39:52, Liquan (Max) Gu wrote: > Thanks @haraken. I am not sure why we use execution context or v8::context > actually, but there is no v8::context for ToCoreProbeSink in coreProbes.h, do we > want to add it? > ExecutionContext is needed for compatibility with PerformanceMonitor, I don't think we need to introduce v8context there: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Per... > https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right): > > https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:131: > ExecutionContext::From(ToScriptStateForMainWorld(GetFrame())), > On 2017/07/17 16:16:24, haraken wrote: > > > > This won't give you a correct context. > > > > You'll need to call ExecutionContext::From(ScriptState::From(context)). > > Thanks! > > https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > https://codereview.chromium.org/2962353002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > ExecutionContext::From(ScriptState::Current(isolate)), source, > On 2017/07/17 16:16:24, haraken wrote: > > > > Alternately can you pass in ExecutionContext* to > > V8ScriptRunner::CompileAndRunInternalScript? > > Done.
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2962353002/#ps140001 (title: "correct executionContext usage")
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 maxlg@chromium.org
LGTM with one request for a follow-up. https://codereview.chromium.org/2962353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right): https://codereview.chromium.org/2962353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:56: static v8::MaybeLocal<v8::Script> CompileScript(ExecutionContext*, In a follow-up CL, would you make a change to pass in ScriptState* to these methods? Then you don't need to pass both ExecutionContext* and Isolate*. We can get ExecutionContext* and Isolate* from ScriptState*.
I am blocked on the failure of DocumentWriteEvaluatorTest. Should I adjust the setup of the test? Please advice. Thanks! https://codereview.chromium.org/2962353002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h (right): https://codereview.chromium.org/2962353002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h:56: static v8::MaybeLocal<v8::Script> CompileScript(ExecutionContext*, On 2017/07/18 05:18:17, haraken wrote: > > In a follow-up CL, would you make a change to pass in ScriptState* to these > methods? Then you don't need to pass both ExecutionContext* and Isolate*. We can > get ExecutionContext* and Isolate* from ScriptState*. > Done. https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: ScriptState::From(isolate->GetCurrentContext()), source, file_name, I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on ScriptState::From(). Is it because the setup of the test hit the assumption of From()? // ScriptState::from() must not be called for a context that does not have // valid embedder data in the embedder field. Here's the stack trace. https://paste.googleplex.com/6174034731466752
haraken@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: ScriptState::From(isolate->GetCurrentContext()), source, file_name, On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on > ScriptState::From(). Is it because the setup of the test hit the assumption of > From()? > > // ScriptState::from() must not be called for a context that does not have > // valid embedder data in the embedder field. > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 Ah, I think that the problem is that DocumentWriteEvaluator doesn't create any ScriptState. I think that a solution would be to change DocumentWriteEvaluator to create a ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is a bit complex so maybe the binding teams should handle this (unless you want to fix yourself :-). yukishiino@: Would you mind helping on this? https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp:63: ScriptState::Current(isolate), Using ScriptState::Current is not nice unless you're pretty sure that you're in a correct ScriptState. Alternately can you pass in the ScriptState to CompileScript?
Patchset #6 (id:200001) has been deleted
On 2017/07/19 05:51:19, haraken wrote: > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > ScriptState::From(isolate->GetCurrentContext()), source, file_name, > On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on > > ScriptState::From(). Is it because the setup of the test hit the assumption of > > From()? > > > > // ScriptState::from() must not be called for a context that does not have > > // valid embedder data in the embedder field. > > > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 > > Ah, I think that the problem is that DocumentWriteEvaluator doesn't create any > ScriptState. > > I think that a solution would be to change DocumentWriteEvaluator to create a > ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is a bit > complex so maybe the binding teams should handle this (unless you want to fix > yourself :-). > > yukishiino@: Would you mind helping on this? > Thanks haraken@:-) It would be helpful if they can assist with it.
https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunnerTest.cpp:63: ScriptState::Current(isolate), On 2017/07/19 05:51:19, haraken wrote: > > Using ScriptState::Current is not nice unless you're pretty sure that you're in > a correct ScriptState. > > Alternately can you pass in the ScriptState to CompileScript? Done.
On 2017/07/19 14:59:57, Liquan (Max) Gu wrote: > On 2017/07/19 05:51:19, haraken wrote: > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > > ScriptState::From(isolate->GetCurrentContext()), source, file_name, > > On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > > > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on > > > ScriptState::From(). Is it because the setup of the test hit the assumption > of > > > From()? > > > > > > // ScriptState::from() must not be called for a context that does not > have > > > // valid embedder data in the embedder field. > > > > > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 > > > > Ah, I think that the problem is that DocumentWriteEvaluator doesn't create any > > ScriptState. > > > > I think that a solution would be to change DocumentWriteEvaluator to create a > > ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is a > bit > > complex so maybe the binding teams should handle this (unless you want to fix > > yourself :-). > > > > yukishiino@: Would you mind helping on this? > > > > > Thanks haraken@:-) It would be helpful if they can assist with it. I'm happy to help this. But I'm not sure if I can secure time this week. Please ping me if this is urgent.
On 2017/07/19 15:15:20, Yuki wrote: > On 2017/07/19 14:59:57, Liquan (Max) Gu wrote: > > On 2017/07/19 05:51:19, haraken wrote: > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > > > ScriptState::From(isolate->GetCurrentContext()), source, file_name, > > > On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > > > > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on > > > > ScriptState::From(). Is it because the setup of the test hit the > assumption > > of > > > > From()? > > > > > > > > // ScriptState::from() must not be called for a context that does not > > have > > > > // valid embedder data in the embedder field. > > > > > > > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 > > > > > > Ah, I think that the problem is that DocumentWriteEvaluator doesn't create > any > > > ScriptState. > > > > > > I think that a solution would be to change DocumentWriteEvaluator to create > a > > > ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is a > > bit > > > complex so maybe the binding teams should handle this (unless you want to > fix > > > yourself :-). > > > > > > yukishiino@: Would you mind helping on this? > > > > > > > > > Thanks haraken@:-) It would be helpful if they can assist with it. > > I'm happy to help this. But I'm not sure if I can secure time this week. > Please ping me if this is urgent. It's not very urgent. Just take your time:-)
On 2017/07/19 15:15:20, Yuki wrote: > On 2017/07/19 14:59:57, Liquan (Max) Gu wrote: > > On 2017/07/19 05:51:19, haraken wrote: > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > > > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > > > ScriptState::From(isolate->GetCurrentContext()), source, file_name, > > > On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > > > > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail on > > > > ScriptState::From(). Is it because the setup of the test hit the > assumption > > of > > > > From()? > > > > > > > > // ScriptState::from() must not be called for a context that does not > > have > > > > // valid embedder data in the embedder field. > > > > > > > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 > > > > > > Ah, I think that the problem is that DocumentWriteEvaluator doesn't create > any > > > ScriptState. > > > > > > I think that a solution would be to change DocumentWriteEvaluator to create > a > > > ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is a > > bit > > > complex so maybe the binding teams should handle this (unless you want to > fix > > > yourself :-). > > > > > > yukishiino@: Would you mind helping on this? > > > > > > > > > Thanks haraken@:-) It would be helpful if they can assist with it. > > I'm happy to help this. But I'm not sure if I can secure time this week. > Please ping me if this is urgent. Max, Is it possible to unblock this CL by moving the ScriptState arg to a follow-up CL (as haraken has originally suggested)?
On 2017/07/19 15:49:56, panicker wrote: > On 2017/07/19 15:15:20, Yuki wrote: > > On 2017/07/19 14:59:57, Liquan (Max) Gu wrote: > > > On 2017/07/19 05:51:19, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2962353002/diff/180001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:597: > > > > ScriptState::From(isolate->GetCurrentContext()), source, file_name, > > > > On 2017/07/18 21:18:50, Liquan (Max) Gu wrote: > > > > > I am blocked on this test. All tests in DocumentWriteEvaluatorTest fail > on > > > > > ScriptState::From(). Is it because the setup of the test hit the > > assumption > > > of > > > > > From()? > > > > > > > > > > // ScriptState::from() must not be called for a context that does > not > > > have > > > > > // valid embedder data in the embedder field. > > > > > > > > > > Here's the stack trace. https://paste.googleplex.com/6174034731466752 > > > > > > > > Ah, I think that the problem is that DocumentWriteEvaluator doesn't create > > any > > > > ScriptState. > > > > > > > > I think that a solution would be to change DocumentWriteEvaluator to > create > > a > > > > ScriptState in DocumentWriteEvaluator::EnsureEvaluationContext(). This is > a > > > bit > > > > complex so maybe the binding teams should handle this (unless you want to > > fix > > > > yourself :-). > > > > > > > > yukishiino@: Would you mind helping on this? > > > > > > > > > > > > > Thanks haraken@:-) It would be helpful if they can assist with it. > > > > I'm happy to help this. But I'm not sure if I can secure time this week. > > Please ping me if this is urgent. > > Max, Is it possible to unblock this CL by moving the ScriptState arg to a > follow-up CL (as haraken has originally suggested)? Oh, sorry I missed it that it should be in followed up CL. I will do that.
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/2962353002/#ps240001 (title: "revert to "correct executionContext usage"")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM assuming that this CL passes all tests. https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp (right): https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: ExecutionContext::From(ScriptState::Current(isolate)), I'm not sure how this works though. I guess ScriptState::Current will return nullptr because DocumentWriteEvaluator is not creating its ScriptState...
The CQ bit was unchecked by maxlg@chromium.org
https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp (right): https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: ExecutionContext::From(ScriptState::Current(isolate)), On 2017/07/19 19:16:20, haraken wrote: > > I'm not sure how this works though. I guess ScriptState::Current will return > nullptr because DocumentWriteEvaluator is not creating its ScriptState... > You are right. Tests of DocumentWriteEvaluatorTest doesn't pass for the same issue.
On 2017/07/19 19:33:49, Liquan (Max) Gu wrote: > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp > (right): > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: > ExecutionContext::From(ScriptState::Current(isolate)), > On 2017/07/19 19:16:20, haraken wrote: > > > > I'm not sure how this works though. I guess ScriptState::Current will return > > nullptr because DocumentWriteEvaluator is not creating its ScriptState... > > > > You are right. Tests of DocumentWriteEvaluatorTest doesn't pass for the same > issue. Yeah. Unfortunately I think we need to address the issue anyway...
On 2017/07/19 19:37:48, haraken wrote: > On 2017/07/19 19:33:49, Liquan (Max) Gu wrote: > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp > > (right): > > > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: > > ExecutionContext::From(ScriptState::Current(isolate)), > > On 2017/07/19 19:16:20, haraken wrote: > > > > > > I'm not sure how this works though. I guess ScriptState::Current will return > > > nullptr because DocumentWriteEvaluator is not creating its ScriptState... > > > > > > > You are right. Tests of DocumentWriteEvaluatorTest doesn't pass for the same > > issue. > > Yeah. Unfortunately I think we need to address the issue anyway... Can we just not pass any context? It was there just for compatibility originally.
On 2017/07/19 19:39:52, Liquan (Max) Gu wrote: > On 2017/07/19 19:37:48, haraken wrote: > > On 2017/07/19 19:33:49, Liquan (Max) Gu wrote: > > > > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: > > > ExecutionContext::From(ScriptState::Current(isolate)), > > > On 2017/07/19 19:16:20, haraken wrote: > > > > > > > > I'm not sure how this works though. I guess ScriptState::Current will > return > > > > nullptr because DocumentWriteEvaluator is not creating its ScriptState... > > > > > > > > > > You are right. Tests of DocumentWriteEvaluatorTest doesn't pass for the same > > > issue. > > > > Yeah. Unfortunately I think we need to address the issue anyway... > > Can we just not pass any context? It was there just for compatibility > originally. panicker: Is it okay to pass nullptr to probe's ExecutionContext?
Patchset #8 (id:260001) has been deleted
On 2017/07/19 19:48:54, haraken wrote: > On 2017/07/19 19:39:52, Liquan (Max) Gu wrote: > > On 2017/07/19 19:37:48, haraken wrote: > > > On 2017/07/19 19:33:49, Liquan (Max) Gu wrote: > > > > > > > > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2962353002/diff/240001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/bindings/core/v8/DocumentWriteEvaluator.cpp:146: > > > > ExecutionContext::From(ScriptState::Current(isolate)), > > > > On 2017/07/19 19:16:20, haraken wrote: > > > > > > > > > > I'm not sure how this works though. I guess ScriptState::Current will > > return > > > > > nullptr because DocumentWriteEvaluator is not creating its > ScriptState... > > > > > > > > > > > > > You are right. Tests of DocumentWriteEvaluatorTest doesn't pass for the > same > > > > issue. > > > > > > Yeah. Unfortunately I think we need to address the issue anyway... > > > > Can we just not pass any context? It was there just for compatibility > > originally. > > panicker: Is it okay to pass nullptr to probe's ExecutionContext? I'll defer to alph@, but fine with me if it helps make incremental progress
I've removed execution context and use nullptr instead.
LGTM BTW if it's fine to pass nullptr to ExecutionContext*, I'd prefer removing the ExecutionContext* parameter though. (You don't need to do that in this CL.)
Liquan, could you kindly elaborate the problem you have again? I patched your CL locally, and ran webkit_unit_tests --gtest_filter='DocumentWriteEvaluatorTest.*', and it passed on a local GNU/Linux machine. What is the problem? You seemed having stopped using ToExecutionContext, and the problem disappeared? If you still need a ScriptState and/or ExecutionContext, is it an option to use V8TestingScope? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... There are many use cases of V8TestingScope and you'll see them through code search. Thanks, Yuki Shiino
LGTM https://codereview.chromium.org/2962353002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right): https://codereview.chromium.org/2962353002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:207: // Todo(maxlg) bug 738495: intentionally leave out as we need to verify Update comment format to: TODO(maxgl): https://crbug.com/738495 ...
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, panicker@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2962353002/#ps300001 (title: "update Todos")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/20 15:26:35, haraken wrote: > LGTM > > BTW if it's fine to pass nullptr to ExecutionContext*, I'd prefer removing the > ExecutionContext* parameter though. (You don't need to do that in this CL.) Do you mean dropping the ExecutionContext in the probe? We probably couldn't do that (correct me if I am wrong panicker@). From what i see, the probe need a executionContext or document to call performanceMonitor, which is eventually used to measure time.
On 2017/07/20 17:35:27, Yuki wrote: > Liquan, could you kindly elaborate the problem you have again? > > I patched your CL locally, and ran webkit_unit_tests > --gtest_filter='DocumentWriteEvaluatorTest.*', and it passed on a local > GNU/Linux machine. What is the problem? You seemed having stopped using > ToExecutionContext, and the problem disappeared? > > If you still need a ScriptState and/or ExecutionContext, is it an option to use > V8TestingScope? > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > There are many use cases of V8TestingScope and you'll see them through code > search. > > Thanks, > Yuki Shiino Hi Yuki, Sorry I removed the part that causes failure in the latest patch so as not blocking this CL. That's why you couldn't see the test failure. I've created another CL for your experiment: https://chromium-review.googlesource.com/c/580401/, what's the previous patch that fails the test, and we will need this CL after your patch is in. I will elaborate the issue in the issue page. Thanks for your time.
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1500581563066510,
"parent_rev": "e1b825587189c7f20e735e15a70433eb90994e63", "commit_rev":
"e51107e73a6f93cfb447d6f5b432c100ef31d309"}
Message was sent while issue was closed.
Description was changed from ========== Add a probe for V8.Complie in order to measure the time of compiling script for V8. BUG=738495 ========== to ========== Add a probe for V8.Complie in order to measure the time of compiling script for V8. BUG=738495 Review-Url: https://codereview.chromium.org/2962353002 Cr-Commit-Position: refs/heads/master@{#488471} Committed: https://chromium.googlesource.com/chromium/src/+/e51107e73a6f93cfb447d6f5b432... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:300001) as https://chromium.googlesource.com/chromium/src/+/e51107e73a6f93cfb447d6f5b432... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
