gin: Make v8_context_snapshot.bin as a default snapshot blob [chromium/src : master]
Hitoshi Yoshida (Gerrit)
Hitoshi Yoshida uploaded patch set #8 to this change.
gin: Make v8_context_snapshot.bin as a default snapshot blob
To make Chrome independent from snapshot_blob.bin, this CL
makes v8_context_snapshot.bin as the default snapshot.
As a background issue, if we want to use JS (=V8), we have to
load a snapshot file on most platforms. It means we have to
load either snapshot_blob.bin or v8_context_snapshot.bin.
And some unit tests, e.g. net_unittests, do not need to use
v8_context_snapshot.bin, and they don't want to depend on
blink component. (It takes very long time just to create the
snapshot.)
This CL makes it possible to load either snapshot file
depending on the order of function calls, and make dependencies
clear.
Bug: 789964
Change-Id: I4df90ed5fe7be37ab969a7f7d5db79bf572ed02a
---
M content/app/content_main_runner.cc
M content/public/test/content_test_suite_base.cc
M content/test/test_blink_web_unit_test_support.cc
M gin/isolate_holder.cc
M gin/public/isolate_holder.h
M gin/v8_initializer.cc
M gin/v8_initializer.h
M media/blink/run_all_unittests.cc
M services/data_decoder/image_decoder_impl_unittest.cc
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.h
11 files changed, 91 insertions(+), 127 deletions(-)
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
I updated this CL and its description.
PTAL.
I'll add some components' owners if Ross and Haraken
agree the design.
1 comment:
File gin/public/isolate_holder.h:
Patch Set #6, Line 127:
AccessMode access_mode_;I'm not quite sure why we need this startup_data_ in IsolateHolder? Don't we just want a flag passed […]
Sounds good to use the same pointer.
I was thinking this CL was a refactoring, but it means nothing. What we actually want to do is what you commented.
Thanks.
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Ross McIlroy (Gerrit)
Overall I like it. My main suggestion would be could we avoid having duplicate functions for loading / initializing the normal snapshot vs the context snapshot and instead pass an enum as an argument to a single snapshot initialization function.
4 comments:
File content/app/content_main_runner.cc:
Patch Set #8, Line 200:
kV8SnapshotDataDescriptorGiven these functions are almost identical, can we just have a single LoadV8SnapshotFile and #ifdef the decision of whether we load kV8SnapshotDataDescriptor or kV8ContextSnapshotDataDescriptor or call gin::V8Initializer::LoadV8Snapshot or gin::V8Initializer::LoadV8ContextSnapshot()?
File gin/public/isolate_holder.h:
enum class IsolateCreator {
kIsolateHolder,
kSnapshotCreator,
};The isolate is always held / created by an IsolateHolder so I find kIsolateHolder a bit confusing. How about:
enum class IsolateUser {
kNormal,
kSnapshotCreator,
}
Patch Set #8, Line 243:
DCHECK(g_mapped_snapshot);This is a change in behavior - in the previous code if the snapshot file fails to load for some reason we can still run Chrome, but now we crash. Could you change it back to the previous code and followup with this change if you want to clean it up (and are sure it's safe) in a followup CL?
Patch Set #8, Line 308:
LoadV8ContextSnapshotCan we have some logic to make sure the caller only calls one of LoadV8ContextSnapshot or LoadV8Snapshot, or perhaps we could replace all the LoadV8ContextSnapshot type functions with just LoadV8Snapshot which has an enum passed as an argument to specify whether to load the context snapshot or the normal one (defaulting to the normal one) - that way we can just check that it is always called with the same enum by a given process.
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
Hitoshi Yoshida would like Ken Rockot, Frank Liberato and Kinuko Yasuda to review this change.
gin: Make v8_context_snapshot.bin as a default snapshot blob
To make Chrome independent from snapshot_blob.bin, this CL
makes v8_context_snapshot.bin as the default snapshot.
As a background issue, if we want to use JS (=V8), we have to
load a snapshot file on most platforms. It means we have to
load either snapshot_blob.bin or v8_context_snapshot.bin.
And some unit tests, e.g. net_unittests, do not need to use
v8_context_snapshot.bin, and they don't want to depend on
blink component. (It takes very long time just to create the
snapshot.)
This CL makes it possible to load either snapshot file
depending on the order of function calls, and make dependencies
clear.
Bug: 789964
Change-Id: I4df90ed5fe7be37ab969a7f7d5db79bf572ed02a
---
M content/app/BUILD.gn
M content/app/content_main_runner.cc
M content/public/test/content_test_suite_base.cc
M content/shell/BUILD.gn
M content/test/test_blink_web_unit_test_support.cc
M gin/isolate_holder.cc
M gin/public/isolate_holder.h
M gin/v8_initializer.cc
M gin/v8_initializer.h
M media/blink/run_all_unittests.cc
M services/data_decoder/image_decoder_impl_unittest.cc
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.h
13 files changed, 89 insertions(+), 142 deletions(-)
Hitoshi Yoshida (Gerrit)
+R:
kinuko for content/
rockot for services/
liberato for media/
PTAL
3 comments:
Given these functions are almost identical, can we just have a single LoadV8SnapshotFile and #ifdef […]
Done
File gin/public/isolate_holder.h:
enum class IsolateUser {
kNormal,
kSnapshotCreator,
};The isolate is always held / created by an IsolateHolder so I find kIsolateHolder a bit confusing. […]
Done
Patch Set #8, Line 243:
v8::StartupData natives;This is a change in behavior - in the previous code if the snapshot file fails to load for some reas […]
You are correct. I thought Chrome would crash in v8::IsolateNewImpl() without SetSnapshotDataBlob(), but it should go well.
I reverted it, but did a small trivial refactoring.
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
//content lgtm
Patch set 9:Code-Review +1
Ross McIlroy (Gerrit)
Overall looks much better, thanks, but still one unresolved comment - let me know if it causes issues.
1 comment:
Can we have some logic to make sure the caller only calls one of LoadV8ContextSnapshot or LoadV8Snap […]
This is still not resolved, I still think it would be better to have a single LoadV8Snapshot which is passed an enum to decide which snapshot should be loaded (and DCHECK if a previous call tried to load the other snapshot).
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
1 comment:
This is still not resolved, I still think it would be better to have a single LoadV8Snapshot which i […]
Oops, I overlooked this.
But I prefer not having such DCHECK() for some unit tests.For example, extensions_unittests does not require customized v8::Context in v8_context_snapshot.bin in its test implementations, so it calls LoadV8Snapshot(kDefault)
in extensions::APIBindingTest::SetUp().
However, because of class inheritances, it also calls LoadV8Snapshot(kV8Context)
first in content::TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport.I think it is not good to force extensions_unittests to depend on v8_context_snapshot,
which can result in the same problem with net_unittests in our discussion.
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Ken Rockot (Gerrit)
LGTM for services/
Patch set 10:Code-Review +1
Ross McIlroy (Gerrit)
Overall LGTM once comments are addresed. I'd still like the DCHECK to be added, but am happy for that to be done as a followup CL to unblock this CL.
8 comments:
File content/public/test/content_test_suite_base.cc:
Patch Set #10, Line 45:
gin::V8Initializer::SnapshotFileType::kV8Contextnit - could you pull this out as a constant - e.g., kSnapshotType, which is defined based on USE_V8_CONTEXT_SNAPSHOT and call LoadV8Snapshot(kSnapshotType) here only once.
File content/test/test_blink_web_unit_test_support.cc:
Patch Set #10, Line 139:
SnapshotFileTypeditto
|kV8Context| figures the file which contains some snapshots of
// customized v8::Context for Blink.Please this to be inline comments in the enum next to kV8Context and update to read:
"Snapshot augmented with an additional context specific snapshot, which can be deserialized using v8::Context::FromSnapshot"Patch Set #10, Line 40:
SnapshotFileTypeV8SnapshotFileType
Patch Set #10, Line 42:
kV8ContextkContext
Oops, I overlooked this. […]
Thanks for making the enum change, however I still think we should have the DCHECK (though I'm fine with doing this in a followup CL where we can clear up these dependencies) - I don't think the reasoning above holds - if extensions::APIBindingTest is a subclass of content::TestBlinkWebUnitTestSupport, then it is fundamentally dependent on Blink, and therefore also dependent on v8_context_snapshot. This was not the case with net_unittests, which I agree should not depend on v8_context_snapshot, however it should only ever call LoadV8Snapshot(kDefault) if it doesn't depend on Blink.
Could you investigate adding this DCHECK in a followup CL please.
File media/blink/run_all_unittests.cc:
Patch Set #10, Line 92:
SnapshotFileTypeditto
File services/data_decoder/image_decoder_impl_unittest.cc:
Patch Set #10, Line 80:
gin::V8Initializer::SnapshotFileType::kDefault);ditto
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
media/ lgtm, with or without my suggestion.
thanks
-fl
1 comment:
File content/public/test/content_test_suite_base.cc:
#if defined(V8_USE_EXTERNAL_STARTUP_DATA)
#if defined(USE_V8_CONTEXT_SNAPSHOT)
gin::V8Initializer::LoadV8Snapshot(
gin::V8Initializer::SnapshotFileType::kV8Context);
#else
gin::V8Initializer::LoadV8Snapshot(
gin::V8Initializer::SnapshotFileType::kDefault);
#endif
gin::V8Initializer::LoadV8Natives();
#endifthis block of code seems to show up a lot. is it worthwhile to pull the whole thing out into a single function / macro / class / whatever?
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
Patch set 11:Commit-Queue +1
9 comments:
File content/public/test/content_test_suite_base.cc:
nit - could you pull this out as a constant - e.g. […]
Done
gin::V8Initializer::V8SnapshotFileType::kDefault;
#endif
#endif
}
ContentTestSuiteBase::ContentTestSuiteBase(int argc, char** argv)
: base::TestSuite(argc, argv) {
}
void Cthis block of code seems to show up a lot. […]
Pulling out the constants in Ross's comment simplify the block, and I think it is simple enough to keep here.
File content/test/test_blink_web_unit_test_support.cc:
ditto
Done
m class V8SnapshotFileType {
kDefault,Please this to be inline comments in the enum next to kV8Context and update to read: […]
Done
V8SnapshotFileType
Done
Patch Set #10, Line 42:
// can bekContext
Done
Thanks for making the enum change, however I still think we should have the DCHECK (though I'm fine […]
Yes, it must be done. Filed https://crbug.com/802962
File media/blink/run_all_unittests.cc:
ditto
Done
File services/data_decoder/image_decoder_impl_unittest.cc:
ditto
Done
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
LGTM
Patch set 11:Code-Review +1
3 comments:
File gin/public/isolate_holder.h:
enum class IsolateCreationMode {
kCreateSnapshot,
kUseSnapshot,
};?
Patch Set #11, Line 43:
kContext,kWithExternalContext ?
File third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp:
Patch Set #11, Line 84:
// This constructor is used for taking a V8 context snapshot. It must run on thetaking => creating
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
3 comments:
enum class IsolateCreationMode { […]
Sounds better, but I'd keep |kNormal| because we can create Isolate without any snapshot files.
Patch Set #11, Line 43:
kWithAdditionalContext,kWithExternalContext ?
Done
File third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp:
Patch Set #11, Line 84:
// This constructor is used for creating a V8 context snapshot. It must run ontaking => creating
done. We may also rename V8ContextSnapshotMode::kTakeSnapshot and similar names, but I'll do it in another CL.
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
let me try to commit, though I'm not sure CQ accepts it...
Patch set 12:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Work for comments" https://chromium-review.googlesource.com/c/859577/12
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/859577/12
Bot data: {"action": "start", "triggered_at": "2018-01-18T10:05:44.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "bcaf9a71150f616229043710e9290f4047a7f2f7"}
Commit Bot (Gerrit)
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_presubmit/builds/638699)
Hitoshi Yoshida (Gerrit)
Ross, ping?
This needs your +1 to land.
Ross McIlroy (Gerrit)
Sorry missed pressing the +1 button :).
Patch set 12:Code-Review +1
Hitoshi Yoshida (Gerrit)
Hitoshi Yoshida uploaded patch set #13 to this change.
gin: Make v8_context_snapshot.bin as a default snapshot blob
To make Chrome independent from snapshot_blob.bin, this CL
makes v8_context_snapshot.bin as the default snapshot.
As a background issue, if we want to use JS (=V8), we have to
load a snapshot file on most platforms. It means we have to
load either snapshot_blob.bin or v8_context_snapshot.bin.
And some unit tests, e.g. net_unittests, do not need to use
v8_context_snapshot.bin, and they don't want to depend on
blink component. (It takes very long time just to create the
snapshot.)
This CL makes it possible to load either snapshot file
depending on the order of function calls, and make dependencies
clear.
TBR=liberato
Bug: 789964
Change-Id: I4df90ed5fe7be37ab969a7f7d5db79bf572ed02a
---
M content/app/BUILD.gn
M content/app/content_main_runner.cc
M content/public/test/content_test_suite_base.cc
M content/shell/BUILD.gn
M content/test/test_blink_web_unit_test_support.cc
M gin/isolate_holder.cc
M gin/public/isolate_holder.h
M gin/v8_initializer.cc
M gin/v8_initializer.h
M media/blink/run_all_unittests.cc
M services/data_decoder/image_decoder_impl_unittest.cc
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.h
13 files changed, 163 insertions(+), 196 deletions(-)
To view, visit change 859577. To unsubscribe, or for help writing mail filters, visit settings.
Hitoshi Yoshida (Gerrit)
Thank you. No problem. :)
Patch set 13:Code-Review +1Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/859577/13
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/859577/13
Bot data: {"action": "start", "triggered_at": "2018-01-19T15:42:58.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "492ec1aa0ae95541744820d26afe4e461854983d"}
Commit Bot (Gerrit)
Commit Bot merged this change.
gin: Make v8_context_snapshot.bin as a default snapshot blob
To make Chrome independent from snapshot_blob.bin, this CL
makes v8_context_snapshot.bin as the default snapshot.
As a background issue, if we want to use JS (=V8), we have to
load a snapshot file on most platforms. It means we have to
load either snapshot_blob.bin or v8_context_snapshot.bin.
And some unit tests, e.g. net_unittests, do not need to use
v8_context_snapshot.bin, and they don't want to depend on
blink component. (It takes very long time just to create the
snapshot.)
This CL makes it possible to load either snapshot file
depending on the order of function calls, and make dependencies
clear.
TBR=liberato
Bug: 789964
Change-Id: I4df90ed5fe7be37ab969a7f7d5db79bf572ed02a
Reviewed-on: https://chromium-review.googlesource.com/859577
Reviewed-by: Hitoshi Yoshida <pe...@chromium.org>
Reviewed-by: Ross McIlroy <rmci...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Ken Rockot <roc...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Commit-Queue: Hitoshi Yoshida <pe...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530539}
---
M content/app/BUILD.gn
M content/app/content_main_runner.cc
M content/public/test/content_test_suite_base.cc
M content/shell/BUILD.gn
M content/test/test_blink_web_unit_test_support.cc
M gin/isolate_holder.cc
M gin/public/isolate_holder.h
M gin/v8_initializer.cc
M gin/v8_initializer.h
M media/blink/run_all_unittests.cc
M services/data_decoder/image_decoder_impl_unittest.cc
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.cpp
M third_party/WebKit/Source/platform/bindings/V8PerIsolateData.h
13 files changed, 163 insertions(+), 196 deletions(-)