Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
528013002:
Restore in-memory parser cache for V8 compile. (Closed)
|
Created:
6 years, 3 months ago by vogelheim Modified:
6 years, 3 months ago CC:
abarth-chromium, arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, gavinp+loader_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionRestore in-memory parser cache for V8 compile.
Also do a minor refactoring of the decision logic, which hopefully
makes it a bit easier to understand.
This is a fix for the performance regression introduced by
crrev.com/432273004, which inadvertently disabled the parser cache
when compiling without any compile options.
R=marja@chromium.org
BUG=404622
BUG=399580
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181472
Patch Set 1 : #
Total comments: 6
Patch Set 2 : review feedback #Patch Set 3 : Add unit tests. #
Total comments: 7
Patch Set 4 : bool -> enum. #
Total comments: 22
Messages
Total messages: 33 (7 generated)
Patchset #1 (id:1) has been deleted
Yay, the in-memory cache is back!
haraken@chromium.org changed reviewers: + haraken@chromium.org
I hope we have a unit test for this feature to prevent future regression. The state machine of the compile cache is complex and hard to verify the behavior without testing. https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:65: v8::Local<v8::Script> CompileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, bool persistCache) CompileAndProduceCache => compileAndProduceCache
I'm not sure whether a unit test is reasonable... this is about configuration (at the moment, we want to configure blink in such a way that parser cache is produced and stored in memory, but it's imaginable that in the future we want to configure blink in some other way). It's hard to unit test configuration in a meaningful way. When somebody changes the configuration system, they'd also need to rewrite the unit test (i.e., we don't have many scenarios where we'd only modify the code and the unit test would help us ensure we didn't break anything). However, I agree that it would be good to have *something* (not necessarily a unit test) that goes red when this breaks - and normally we do; there was a bug in the perf alert system and that's why vogelheim@ didn't get an alert. vogelheim@, wdyt, is it possible to add a meaningful test here? https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:123: // Device on the compile options (caching). "Device on"? (Obvsly I'm not a native speaker but what does "device on" mean?) https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:154: switch (compileScriptAction) { Sounds a bit duplicate to first gather all the information in compileScriptAction first, and right after that, split it to "cache option" & "persist" & which function to call. Can you do this in one go? Can't the switch above call the right function with the right parameters? Does it get messy if you do that?
On 2014/09/03 07:17:47, marja wrote: > I'm not sure whether a unit test is reasonable... this is about configuration > (at the moment, we want to configure blink in such a way that parser cache is > produced and stored in memory, but it's imaginable that in the future we want to > configure blink in some other way). It's hard to unit test configuration in a > meaningful way. > > When somebody changes the configuration system, they'd also need to rewrite the > unit test (i.e., we don't have many scenarios where we'd only modify the code > and the unit test would help us ensure we didn't break anything). > > However, I agree that it would be good to have *something* (not necessarily a > unit test) that goes red when this breaks - and normally we do; there was a bug > in the perf alert system and that's why vogelheim@ didn't get an alert. > > vogelheim@, wdyt, is it possible to add a meaningful test here? Would it be helpful to create some mock for the configuration system and use it in a unit test?
On 2014/09/03 07:19:53, haraken wrote: > On 2014/09/03 07:17:47, marja wrote: > > I'm not sure whether a unit test is reasonable... this is about configuration > > (at the moment, we want to configure blink in such a way that parser cache is > > produced and stored in memory, but it's imaginable that in the future we want > to > > configure blink in some other way). It's hard to unit test configuration in a > > meaningful way. > > > > When somebody changes the configuration system, they'd also need to rewrite > the > > unit test (i.e., we don't have many scenarios where we'd only modify the code > > and the unit test would help us ensure we didn't break anything). > > > > However, I agree that it would be good to have *something* (not necessarily a > > unit test) that goes red when this breaks - and normally we do; there was a > bug > > in the perf alert system and that's why vogelheim@ didn't get an alert. > > > > vogelheim@, wdyt, is it possible to add a meaningful test here? > > Would it be helpful to create some mock for the configuration system and use it > in a unit test? I don't know what kind of mock you mean specifically, but the danger is that it becomes meaningless when the implementation changes, and needs to be rewritten too. If we want to test the configuration system, we should mock things which are interfacing against it, not the configuration system itself (doesn't make sense to mock the entity to be tested)... The most obvious way to break this feature is to rewrite the configuration system (like dvogelheim@ just did ;) so that's the breakage we should be guarding against. If a test needs to be rewritten when the configuration system is rewritten, it's less useful. (It will only serve as a reminder that there is this feature, but it's easy to accidentally rewrite the test to pass with the new broken configuration system.)
On 2014/09/03 07:24:36, marja wrote: > On 2014/09/03 07:19:53, haraken wrote: > > On 2014/09/03 07:17:47, marja wrote: > > > I'm not sure whether a unit test is reasonable... this is about > configuration > > > (at the moment, we want to configure blink in such a way that parser cache > is > > > produced and stored in memory, but it's imaginable that in the future we > want > > to > > > configure blink in some other way). It's hard to unit test configuration in > a > > > meaningful way. > > > > > > When somebody changes the configuration system, they'd also need to rewrite > > the > > > unit test (i.e., we don't have many scenarios where we'd only modify the > code > > > and the unit test would help us ensure we didn't break anything). > > > > > > However, I agree that it would be good to have *something* (not necessarily > a > > > unit test) that goes red when this breaks - and normally we do; there was a > > bug > > > in the perf alert system and that's why vogelheim@ didn't get an alert. > > > > > > vogelheim@, wdyt, is it possible to add a meaningful test here? > > > > Would it be helpful to create some mock for the configuration system and use > it > > in a unit test? > > I don't know what kind of mock you mean specifically, but the danger is that it > becomes meaningless when the implementation changes, and needs to be rewritten > too. > > If we want to test the configuration system, we should mock things which are > interfacing against it, not the configuration system itself (doesn't make sense > to mock the entity to be tested)... > > The most obvious way to break this feature is to rewrite the configuration > system (like dvogelheim@ just did ;) so that's the breakage we should be > guarding against. If a test needs to be rewritten when the configuration system > is rewritten, it's less useful. (It will only serve as a reminder that there is > this feature, but it's easy to accidentally rewrite the test to pass with the > new broken configuration system.) hmm, I understand what you mean... It would be great if there is a way to catch a regression on this feature though.
Normally is caught by the morejs warm times regression - just this time it wasn't detected because the perf alert system was broken. However, it would be nice to catch it *earlier* than the perf alert system does, so I'm all for it if somebody has an idea how to test this in a meaningful way.
https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:65: v8::Local<v8::Script> CompileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, bool persistCache) On 2014/09/03 04:11:43, haraken wrote: > > CompileAndProduceCache => compileAndProduceCache Done. https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:123: // Device on the compile options (caching). On 2014/09/03 07:17:47, marja wrote: > "Device on"? (Obvsly I'm not a native speaker but what does "device on" mean?) I meant: "Decide on ...". Not sure what happened there. If had written this on my phone, I'd blame autocorrect. https://codereview.chromium.org/528013002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunner.cpp:154: switch (compileScriptAction) { On 2014/09/03 07:17:47, marja wrote: > Sounds a bit duplicate to first gather all the information in > compileScriptAction first, and right after that, split it to "cache option" & > "persist" & which function to call. > > Can you do this in one go? Can't the switch above call the right function with > the right parameters? Does it get messy if you do that? Hmm. Done. That was meant to separate the decision from the action, but I admit it didn't really buy much. Please have a look at whether you like the new version better.
Patchset #2 (id:40001) has been deleted
lgtm
I'm undecided on the unit test thing... I certainly see the point of adding tests for things that have broken before. The cons I'm seeing are: - I don't see unit tests for anything else in the bindings. Maybe I haven't found them, but that tends to be major alarm signal that the code may not be particularly suitable for unit testing. - Tests work best if there's some invariant you can meaningfully test. That is, if there is a definite, correct answer (or a correct aspect of all answers) to something. But here, we are actually not sure what the correct answer is. The plan is to implement those alternatives; then run benchmarks; and then decide on the new defaults and what areas would benefit from special handling. So whatever is 'correct' now is likely to be incorrect in the next round of updates. - The 'true' test of this functionality would be benchmarks running on bots, that make sure parse + compile time do not regress, regardless of implementation strategy. That is on my agenda, but not done yet. So... if we do want to go the unit test route.. Any examples? And suggestions for what the unit test would be testing? Daniel On Wed, Sep 3, 2014 at 1:21 PM, <vogelheim@chromium.org> wrote: > > https://codereview.chromium.org/528013002/diff/20001/ > Source/bindings/core/v8/V8ScriptRunner.cpp > File Source/bindings/core/v8/V8ScriptRunner.cpp (right): > > https://codereview.chromium.org/528013002/diff/20001/ > Source/bindings/core/v8/V8ScriptRunner.cpp#newcode65 > Source/bindings/core/v8/V8ScriptRunner.cpp:65: v8::Local<v8::Script> > CompileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin > origin, ScriptResource* resource, v8::Isolate* isolate, > v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, bool > persistCache) > On 2014/09/03 04:11:43, haraken wrote: > > CompileAndProduceCache => compileAndProduceCache >> > > Done. > > > https://codereview.chromium.org/528013002/diff/20001/ > Source/bindings/core/v8/V8ScriptRunner.cpp#newcode123 > Source/bindings/core/v8/V8ScriptRunner.cpp:123: // Device on the compile > options (caching). > On 2014/09/03 07:17:47, marja wrote: > >> "Device on"? (Obvsly I'm not a native speaker but what does "device >> > on" mean?) > > I meant: "Decide on ...". Not sure what happened there. If had written > this on my phone, I'd blame autocorrect. > > > https://codereview.chromium.org/528013002/diff/20001/ > Source/bindings/core/v8/V8ScriptRunner.cpp#newcode154 > Source/bindings/core/v8/V8ScriptRunner.cpp:154: switch > (compileScriptAction) { > On 2014/09/03 07:17:47, marja wrote: > >> Sounds a bit duplicate to first gather all the information in >> compileScriptAction first, and right after that, split it to "cache >> > option" & > >> "persist" & which function to call. >> > > Can you do this in one go? Can't the switch above call the right >> > function with > >> the right parameters? Does it get messy if you do that? >> > > Hmm. Done. That was meant to separate the decision from the action, but > I admit it didn't really buy much. Please have a look at whether you > like the new version better. > > https://codereview.chromium.org/528013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/09/03 11:33:04, vogelheim wrote: > I'm undecided on the unit test thing... I certainly see the point of adding > tests for things that have broken before. > > The cons I'm seeing are: > > - I don't see unit tests for anything else in the bindings. Maybe I haven't > found them, but that tends to be major alarm signal that the code may not > be particularly suitable for unit testing. > - Tests work best if there's some invariant you can meaningfully test. That > is, if there is a definite, correct answer (or a correct aspect of all > answers) to something. But here, we are actually not sure what the correct > answer is. The plan is to implement those alternatives; then run > benchmarks; and then decide on the new defaults and what areas would > benefit from special handling. So whatever is 'correct' now is likely to be > incorrect in the next round of updates. > - The 'true' test of this functionality would be benchmarks running on > bots, that make sure parse + compile time do not regress, regardless of > implementation strategy. That is on my agenda, but not done yet. > > So... if we do want to go the unit test route.. Any examples? And > suggestions for what the unit test would be testing? You can grep *Test.cpp in bindings/.
Added unit tests. Tests look reasonable now, but took quite a bit of time to get there. One could certainly test more, but I'd prefer to leave it at that.
The CQ bit was checked by vogelheim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vogelheim@chromium.org/528013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
jochen@chromium.org changed reviewers: + jochen@chromium.org
you need to add the test to the resp. gyp file first https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:42: return v8::Isolate::GetCurrent(); i'm a bit surprised this works. Usually, v8 should get configured somewhere (v8::Platform() put in place etc..) https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... File Source/core/fetch/Resource.h (right): https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... Source/core/fetch/Resource.h:201: void setCachedMetadata(unsigned dataTypeID, const char*, size_t, bool persist = true); is it possible to use a enum instead of the bool?
On 2014/09/05 11:48:04, jochen wrote: > you need to add the test to the resp. gyp file first That's that the change in v8.gypi is meant to do. I did verify that the test case is actually run as part of webkit_unit_tests.
https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:42: return v8::Isolate::GetCurrent(); On 2014/09/05 11:48:04, jochen wrote: > i'm a bit surprised this works. Usually, v8 should get configured somewhere > (v8::Platform() put in place etc..) I copied that from other *Test.cpp in this directory. I assume the setup occurs in the test driver, but honestly I didn't find it. https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... File Source/core/fetch/Resource.h (right): https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... Source/core/fetch/Resource.h:201: void setCachedMetadata(unsigned dataTypeID, const char*, size_t, bool persist = true); On 2014/09/05 11:48:04, jochen wrote: > is it possible to use a enum instead of the bool? Hm. Yes, but why? I don't understand the benefit.
https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:42: return v8::Isolate::GetCurrent(); On 2014/09/05 11:54:12, vogelheim wrote: > On 2014/09/05 11:48:04, jochen wrote: > > i'm a bit surprised this works. Usually, v8 should get configured somewhere > > (v8::Platform() put in place etc..) > > I copied that from other *Test.cpp in this directory. I assume the setup occurs > in the test driver, but honestly I didn't find it. ok, i guess i should clean this up at some point https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... File Source/core/fetch/Resource.h (right): https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... Source/core/fetch/Resource.h:201: void setCachedMetadata(unsigned dataTypeID, const char*, size_t, bool persist = true); On 2014/09/05 11:54:12, vogelheim wrote: > On 2014/09/05 11:48:04, jochen wrote: > > is it possible to use a enum instead of the bool? > > Hm. Yes, but why? I don't understand the benefit. so the callsites are easier to read "setCachedMetaData(..., PersistOnDisc) vs (..., true)
PTAL. Thanks. https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... File Source/core/fetch/Resource.h (right): https://codereview.chromium.org/528013002/diff/80001/Source/core/fetch/Resour... Source/core/fetch/Resource.h:201: void setCachedMetadata(unsigned dataTypeID, const char*, size_t, bool persist = true); Done. I called it CacheLocally and SendToPlatform, though, since technically Blink doesn't know what sort of magic the blink::Platform implementation wants to do with the data. On 2014/09/05 14:05:40, jochen wrote: > On 2014/09/05 11:54:12, vogelheim wrote: > > On 2014/09/05 11:48:04, jochen wrote: > > > is it possible to use a enum instead of the bool? > > > > Hm. Yes, but why? I don't understand the benefit. > > so the callsites are easier to read "setCachedMetaData(..., PersistOnDisc) vs > (..., true)
lgtm.
The CQ bit was checked by vogelheim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vogelheim@chromium.org/528013002/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as 181472
Message was sent while issue was closed.
Sorry about the review delay. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:65: v8::Local<v8::Script> compileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, Resource::MetadataCacheType cacheType) Move the Isolate* to the first parameter. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:132: case V8CacheOptionsOff: How about just putting the 'case V8CacheOptionsOff:' to the next line of 'case V8CacheOptionsParse:'? Those two are doing the same thing (at the moment). https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:135: // TODO(vogelheim): Determine whether this should get its own TODO(vogelheim) => FIXME https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:23: cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2; It is error-prone to calculate the tag value in each call site. Can we create a helper method that returns tagForParserCache() and tagForCodeCache()? https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:40: v8::Isolate* isolate() You should use V8TestingScope. See how it's used in *Test.cpp in bindings/. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:44: v8::Local<v8::String> v8String(const char* value) Can't you use a helper method in V8Binding.h? https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:49: v8::Local<v8::String> v8String(const WTF::String& value) Ditto. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:73: v8::Context::Scope contextScope(context); You can avoid these by using V8TestingScope. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:92: protected: This should be private. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:150: // TODO(vogelheim): Code caching is presently still disabled. TODO(vogelheim) => FIXME
Message was sent while issue was closed.
Will send follow-on CL with the actual fixes, since this one is already committed. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:65: v8::Local<v8::Script> compileAndProduceCache(v8::Handle<v8::String> code, v8::ScriptOrigin origin, ScriptResource* resource, v8::Isolate* isolate, v8::ScriptCompiler::CompileOptions options, unsigned cacheTag, Resource::MetadataCacheType cacheType) On 2014/09/05 16:16:13, haraken wrote: > > Move the Isolate* to the first parameter. Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:132: case V8CacheOptionsOff: On 2014/09/05 16:16:13, haraken wrote: > > How about just putting the 'case V8CacheOptionsOff:' to the next line of 'case > V8CacheOptionsParse:'? Those two are doing the same thing (at the moment). This is unlikely to stay that way. I'd prefer not to. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunner.cpp:135: // TODO(vogelheim): Determine whether this should get its own On 2014/09/05 16:16:13, haraken wrote: > > TODO(vogelheim) => FIXME Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:23: cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2; On 2014/09/05 16:16:13, haraken wrote: > > It is error-prone to calculate the tag value in each call site. Can we create a > helper method that returns tagForParserCache() and tagForCodeCache()? Not sure what you mean: Helper methods in the test fixture (instead of static members), or helper methods in V8ScriptRunner? I've done the former. I don't want to do the latter, since I don't want to use the class-under-test to verify whether the test assertions are correct. That might cause false positives, whenever the same bug affects both the test execution and the test assertion in the same fashion. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:40: v8::Isolate* isolate() On 2014/09/05 16:16:13, haraken wrote: > > You should use V8TestingScope. See how it's used in *Test.cpp in bindings/. Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:44: v8::Local<v8::String> v8String(const char* value) On 2014/09/05 16:16:13, haraken wrote: > > Can't you use a helper method in V8Binding.h? Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:49: v8::Local<v8::String> v8String(const WTF::String& value) On 2014/09/05 16:16:13, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:73: v8::Context::Scope contextScope(context); On 2014/09/05 16:16:13, haraken wrote: > > You can avoid these by using V8TestingScope. Done. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:92: protected: On 2014/09/05 16:16:13, haraken wrote: > > This should be private. No, it should be protected, so that the helper classes generated by the TEST_F macro can use them to verify their test assertions. That's the reason for these members even being here! https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:150: // TODO(vogelheim): Code caching is presently still disabled. On 2014/09/05 16:16:13, haraken wrote: > > TODO(vogelheim) => FIXME Done.
Message was sent while issue was closed.
https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:23: cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2; On 2014/09/05 17:47:29, vogelheim wrote: > On 2014/09/05 16:16:13, haraken wrote: > > > > It is error-prone to calculate the tag value in each call site. Can we create > a > > helper method that returns tagForParserCache() and tagForCodeCache()? > > Not sure what you mean: Helper methods in the test fixture (instead of static > members), or helper methods in V8ScriptRunner? > > I've done the former. > > I don't want to do the latter, since I don't want to use the class-under-test to > verify whether the test assertions are correct. That might cause false > positives, whenever the same bug affects both the test execution and the test > assertion in the same fashion. I was suggesting to add a helper function to V8ScriptRunner and use the helper function to get the cache tag in both V8ScriptRunner and the test.
Message was sent while issue was closed.
Follow-on CL is crrev.com/549533002. https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right): https://codereview.chromium.org/528013002/diff/100001/Source/bindings/core/v8... Source/bindings/core/v8/V8ScriptRunnerTest.cpp:23: cacheTagParser = StringHash::hash(v8::V8::GetVersion()) * 2; On 2014/09/05 17:49:39, haraken wrote: > On 2014/09/05 17:47:29, vogelheim wrote: > > On 2014/09/05 16:16:13, haraken wrote: > > > > > > It is error-prone to calculate the tag value in each call site. Can we > create > > a > > > helper method that returns tagForParserCache() and tagForCodeCache()? > > > > Not sure what you mean: Helper methods in the test fixture (instead of static > > members), or helper methods in V8ScriptRunner? > > > > I've done the former. > > > > I don't want to do the latter, since I don't want to use the class-under-test > to > > verify whether the test assertions are correct. That might cause false > > positives, whenever the same bug affects both the test execution and the test > > assertion in the same fashion. > > I was suggesting to add a helper function to V8ScriptRunner and use the helper > function to get the cache tag in both V8ScriptRunner and the test. Hmm. I'm not convinced that's a good path since 1, that means the test-under-class is used to verify test assertions, and 2, those methods don't really belong into the V8ScriptRunner public interface. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
