Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962963007:
Use locker when creating snapshot if necessary. (Closed)
DescriptionUse locker when creating snapshot if necessary.
R=vogelheim@chromium.org
Committed: https://crrev.com/32979cf6d88b1d837ac4ba4b1ff6ee87acdba0f4
Cr-Commit-Position: refs/heads/master@{#26964}
Patch Set 1 #
Total comments: 1
Patch Set 2 : simplify test case #Patch Set 3 : fix assertion instead #Messages
Total messages: 17 (4 generated)
https://codereview.chromium.org/962963007/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/962963007/diff/1/src/api.cc#newcode378 src/api.cc:378: result = CreateSnapshotWithinIsolate(isolate, custom_source); I'm confused by this. Admittedly I don't understand how the locker is supposed to work and I'm mostly pattern-matching the code review, so quite possibly the confusion is on my part... I take it the Locker is meant to be used be the embedder. For all I can see, this is the only case in the API where the API instantiates a Locker itself. Also, I can't find the pattern of Locker-ing things if Locker::IsActive elsewhere. Why is this the right thing to do here? Is is because we also create our own Isolate here?
On 2015/03/03 13:18:29, vogelheim wrote: > https://codereview.chromium.org/962963007/diff/1/src/api.cc > File src/api.cc (right): > > https://codereview.chromium.org/962963007/diff/1/src/api.cc#newcode378 > src/api.cc:378: result = CreateSnapshotWithinIsolate(isolate, custom_source); > I'm confused by this. Admittedly I don't understand how the locker is supposed > to work and I'm mostly pattern-matching the code review, so quite possibly the > confusion is on my part... > > I take it the Locker is meant to be used be the embedder. For all I can see, > this is the only case in the API where the API instantiates a Locker itself. > Also, I can't find the pattern of Locker-ing things if Locker::IsActive > elsewhere. Why is this the right thing to do here? Is is because we also create > our own Isolate here? Right. The Locker is meant to be used by the embedder. If a Locker is used for an isolate, it needs to be used for all Isolates. You don't find any example elsewhere because CreateSnapshotDataBlob is the only place where we do the whole package of creating a new Isolate, enter it, and execute code on it. If the embedder uses a Locker, we need to match it here as well. Conversely, if we always use a Locker here, we would force the embedder to always use a Locker as well. So the best thing to do here is to use a Locker if necessary, and don't if not.
lgtm Ah, ok, the "if a Locker is used for an isolate it needs to be used for all Isolates" bit explains it.
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/962963007/#ps20001 (title: "simplify test case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962963007/20001
I don't think this will solve the problem I was having. At the time we call CreateSnapshotDataBlob, Locker hasn't been used yet, but another task can create an isolate with a Locker in parallel. So by the time the HandleScope is created inside CreateSnapshotDataBlob, Locker::IsActive will be true, and we'll hit that assertion. Maybe another solution would be to add a flag to v8::internal::Isolate indicating whether or not locking is expected to be used with each isolate. CreateSnapshotDataBlob would set this flag to false. HandleScope::Initialize could check that flag instead of (or in addition to) Locker::IsActive. Since CreateSnapshotDataBlob is the only thing that creates an isolate internally, there's probably no need to expose this flag in the API.
The CQ bit was unchecked by yangguo@chromium.org
On 2015/03/03 14:59:41, jayconrod wrote: > I don't think this will solve the problem I was having. At the time we call > CreateSnapshotDataBlob, Locker hasn't been used yet, but another task can create > an isolate with a Locker in parallel. So by the time the HandleScope is created > inside CreateSnapshotDataBlob, Locker::IsActive will be true, and we'll hit that > assertion. > > Maybe another solution would be to add a flag to v8::internal::Isolate > indicating whether or not locking is expected to be used with each isolate. > CreateSnapshotDataBlob would set this flag to false. HandleScope::Initialize > could check that flag instead of (or in addition to) Locker::IsActive. Since > CreateSnapshotDataBlob is the only thing that creates an isolate internally, > there's probably no need to expose this flag in the API. I see.
On 2015/03/03 15:16:56, Yang wrote: > On 2015/03/03 14:59:41, jayconrod wrote: > > I don't think this will solve the problem I was having. At the time we call > > CreateSnapshotDataBlob, Locker hasn't been used yet, but another task can > create > > an isolate with a Locker in parallel. So by the time the HandleScope is > created > > inside CreateSnapshotDataBlob, Locker::IsActive will be true, and we'll hit > that > > assertion. > > > > Maybe another solution would be to add a flag to v8::internal::Isolate > > indicating whether or not locking is expected to be used with each isolate. > > CreateSnapshotDataBlob would set this flag to false. HandleScope::Initialize > > could check that flag instead of (or in addition to) Locker::IsActive. Since > > CreateSnapshotDataBlob is the only thing that creates an isolate internally, > > there's probably no need to expose this flag in the API. > > I see. Updated with a different solution.
lgtm Just to double check my understanding: - There's no way for an API user to create an Isolate with serializer_enabled() == true. The corresponding constructor isn't exposed. - There's no internal creation of such an Isolate that gets passed out. - Therefore, this change shouldn't affect any user code, ever.
On 2015/03/03 15:43:45, vogelheim wrote: > lgtm > > Just to double check my understanding: > - There's no way for an API user to create an Isolate with serializer_enabled() > == true. The corresponding constructor isn't exposed. > - There's no internal creation of such an Isolate that gets passed out. > - Therefore, this change shouldn't affect any user code, ever. Correct.
The CQ bit was checked by yangguo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962963007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/32979cf6d88b1d837ac4ba4b1ff6ee87acdba0f4 Cr-Commit-Position: refs/heads/master@{#26964} |
