Bug 219663 - Add a JSC API to allow acquiring the JSLock
Summary: Add a JSC API to allow acquiring the JSLock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-08 18:41 PST by Tadeu Zagallo
Modified: 2020-12-14 12:47 PST (History)
11 users (show)

See Also:


Attachments
Patch (36.29 KB, patch)
2020-12-08 19:13 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (35.56 KB, patch)
2020-12-09 13:09 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (36.90 KB, patch)
2020-12-10 14:40 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2020-12-08 18:41:04 PST
...
Comment 1 Tadeu Zagallo 2020-12-08 19:13:52 PST
Created attachment 415700 [details]
Patch
Comment 2 Saam Barati 2020-12-08 20:10:16 PST
Comment on attachment 415700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415700&action=review

> Source/JavaScriptCore/API/JSLockRef.cpp:51
> +JSLockRef JSLock(JSContextRef ctx)
> +{
> +    if (!ctx) {
> +        ASSERT_NOT_REACHED();
> +        return nullptr;
> +    }
> +    JSGlobalObject* globalObject = toJS(ctx);
> +    return toRef(new JSLockHolder(globalObject));
> +}
> +
> +void JSUnlock(JSLockRef ref)
> +{
> +    JSLockHolder* lock = toJS(ref);
> +    if (!lock) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }
> +    delete lock;
> +}

Why are we mallocing an object for this? Let's just call lock()/unlock() on the JSLock, and return back the JSContextRef or something as the "JSLockRef".
Comment 3 Mark Lam 2020-12-08 20:32:13 PST
Comment on attachment 415700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415700&action=review

>> Source/JavaScriptCore/API/JSLockRef.cpp:51
>> +}
> 
> Why are we mallocing an object for this? Let's just call lock()/unlock() on the JSLock, and return back the JSContextRef or something as the "JSLockRef".

I agree: don't go through the JSLockHolder, just lock / unlock the JSLock directly.  There's also no added advantage in introducing a new JSLockRef abstraction.  It is better to make it clear that the lock is associated with the JSContextRef.  By passing that to JSLock() and JSUnlock(), the API would communicate that association, and avoid all kinds of confusions that may arise should the user used more than 1 JSContextRefs.

> Source/JavaScriptCore/API/JSLockRefPrivate.h:38
> +JS_EXPORT JSLockRef JSLock(JSContextRef);
> +
> +JS_EXPORT void JSUnlock(JSLockRef);

It's a pity that we can't use C++ and implement this as an RAII object.  Anyway, I don't see JSUnlock() used in any of your benchmarks.  It may not matter in terms of the benchmark results, but can you call JSUnlock() in the right places just so it documents the right thing to do?  I can see these benchmarks being consulted as examples of "best practices" in the future, and it would be good to be complete in the example.

> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyC/RichardsMostlyC/richards.c:348
> +        JSLock(ctx);

Why is the locking done after the work is finished?  Why is it not done right after creating the context?

> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyObjC/RichardsMostlyObjC/WorkerTask.m:55
> +        JSLock(context.JSGlobalContextRef);

Why is this doing the locking after the script has been evaluated?  Why not right after the context is created?
Comment 4 Tadeu Zagallo 2020-12-09 07:57:14 PST
Comment on attachment 415700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415700&action=review

>>> Source/JavaScriptCore/API/JSLockRef.cpp:51
>>> +}
>> 
>> Why are we mallocing an object for this? Let's just call lock()/unlock() on the JSLock, and return back the JSContextRef or something as the "JSLockRef".
> 
> I agree: don't go through the JSLockHolder, just lock / unlock the JSLock directly.  There's also no added advantage in introducing a new JSLockRef abstraction.  It is better to make it clear that the lock is associated with the JSContextRef.  By passing that to JSLock() and JSUnlock(), the API would communicate that association, and avoid all kinds of confusions that may arise should the user used more than 1 JSContextRefs.

Agreed it's nicer, I'll update it.

>> Source/JavaScriptCore/API/JSLockRefPrivate.h:38
>> +JS_EXPORT void JSUnlock(JSLockRef);
> 
> It's a pity that we can't use C++ and implement this as an RAII object.  Anyway, I don't see JSUnlock() used in any of your benchmarks.  It may not matter in terms of the benchmark results, but can you call JSUnlock() in the right places just so it documents the right thing to do?  I can see these benchmarks being consulted as examples of "best practices" in the future, and it would be good to be complete in the example.

The problem is that there's no right place to call JSUnlock in these benchmarks since the context is stored in a static variable that's never released.

>> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyC/RichardsMostlyC/richards.c:348
>> +        JSLock(ctx);
> 
> Why is the locking done after the work is finished?  Why is it not done right after creating the context?

Same thing here, I moved it to be clearer, but the costly part is after this.

>> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyObjC/RichardsMostlyObjC/WorkerTask.m:55
>> +        JSLock(context.JSGlobalContextRef);
> 
> Why is this doing the locking after the script has been evaluated?  Why not right after the context is created?

I moved it next to the context creation to be clearer, but it doesn't really matter here since the costly part is calling all the C APIs. This is just initial script evaluation that puts the function we need in the global object.
Comment 5 Geoffrey Garen 2020-12-09 10:55:09 PST
Comment on attachment 415700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415700&action=review

>>> Source/JavaScriptCore/API/JSLockRefPrivate.h:38
>>> +JS_EXPORT JSLockRef JSLock(JSContextRef);
>>> +
>>> +JS_EXPORT void JSUnlock(JSLockRef);
>> 
>> It's a pity that we can't use C++ and implement this as an RAII object.  Anyway, I don't see JSUnlock() used in any of your benchmarks.  It may not matter in terms of the benchmark results, but can you call JSUnlock() in the right places just so it documents the right thing to do?  I can see these benchmarks being consulted as examples of "best practices" in the future, and it would be good to be complete in the example.
> 
> The problem is that there's no right place to call JSUnlock in these benchmarks since the context is stored in a static variable that's never released.

Are we planning to make this API public? If so, we should do API review.

An alternative API that is C-compatible and avoids the problem of forgetting to call unlock is to take a block as an argument. You call lock, invoke the block, and then call unlock. That might be a friendlier API since it's much harder to use wrong.
Comment 6 Tadeu Zagallo 2020-12-09 13:09:38 PST
Created attachment 415793 [details]
Patch
Comment 7 Tadeu Zagallo 2020-12-09 18:32:10 PST
> Are we planning to make this API public? If so, we should do API review.

I think it would be nice to make it public, I'll look into getting that started when this lands.

> An alternative API that is C-compatible and avoids the problem of forgetting
> to call unlock is to take a block as an argument. You call lock, invoke the
> block, and then call unlock. That might be a friendlier API since it's much
> harder to use wrong.

I agree that would be a better API in general, but I still think that it's a very common scenario to just create the context, lock it to the current thread and never unlock it again. But I guess we could always have both APIs.
Comment 8 Geoffrey Garen 2020-12-10 09:54:24 PST
> I agree that would be a better API in general, but I still think that it's a
> very common scenario to just create the context, lock it to the current
> thread and never unlock it again. But I guess we could always have both APIs.

Can you give some examples?

The examples I've seen most, at least inside Apple, use dispatch queues. An unfortunate side-effect of a dispatch queue is that it may execute on any thread, so you need to re-aqcuire the JSLock.
Comment 9 Tadeu Zagallo 2020-12-10 13:33:09 PST
(In reply to Geoffrey Garen from comment #8)
> > I agree that would be a better API in general, but I still think that it's a
> > very common scenario to just create the context, lock it to the current
> > thread and never unlock it again. But I guess we could always have both APIs.
> 
> Can you give some examples?
> 
> The examples I've seen most, at least inside Apple, use dispatch queues. An
> unfortunate side-effect of a dispatch queue is that it may execute on any
> thread, so you need to re-aqcuire the JSLock.

Any time you have dedicated thread. Isn't that what we do? Do you see any downside to having both APIs?
Comment 10 Mark Lam 2020-12-10 13:56:50 PST
Comment on attachment 415793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415793&action=review

> Source/JavaScriptCore/API/JSLockRefPrivate.h:38
> + @discussion The lock has to be held to perform any interactions with the JSContextRef. This function allows holding the lock across multiple interactions to amortize the cost.

Should we say anything about the lock being re-entrant?

> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyC/RichardsMostlyC/richards.c:16
> +#import <JavaScriptCore/JSLockRefPrivate.h>
> +#import <JavaScriptCore/JavaScriptCore.h>
>  #import <QuartzCore/QuartzCore.h>

Should this file "RichardsMostlyC" be named richards.m since it is using ObjC constructs?
Comment 11 Tadeu Zagallo 2020-12-10 14:16:17 PST
Comment on attachment 415793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415793&action=review

>> Source/JavaScriptCore/API/JSLockRefPrivate.h:38
>> + @discussion The lock has to be held to perform any interactions with the JSContextRef. This function allows holding the lock across multiple interactions to amortize the cost.
> 
> Should we say anything about the lock being re-entrant?

Sounds like a good idea. I'll add it.

>> PerformanceTests/APIBench/UpcomingAPI/RichardsMostlyC/RichardsMostlyC/richards.c:16
>>  #import <QuartzCore/QuartzCore.h>
> 
> Should this file "RichardsMostlyC" be named richards.m since it is using ObjC constructs?

it's only using #import, I'll change it to #include.
Comment 12 Tadeu Zagallo 2020-12-10 14:40:28 PST
Created attachment 415933 [details]
Patch for landing
Comment 13 Geoffrey Garen 2020-12-10 15:04:21 PST
> > The examples I've seen most, at least inside Apple, use dispatch queues. An
> > unfortunate side-effect of a dispatch queue is that it may execute on any
> > thread, so you need to re-aqcuire the JSLock.
> 
> Any time you have dedicated thread. Isn't that what we do?

Yes, WebKit uses dedicated VMs for the main thread and worker threads. But I haven't seen that kind of use of JSC outside WebKit, in a public API client.

> Do you see any downside to having both APIs?

Not a huge downside, but having two ways to do something pushes onto each API client a certain amount of cognitive load: They need to read documentation, weigh tradeoffs, understand subtleties, and make a decision any time they do that thing.
Comment 14 EWS 2020-12-10 15:11:02 PST
Committed r270659: <https://trac.webkit.org/changeset/270659>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415933 [details].
Comment 15 Radar WebKit Bug Importer 2020-12-10 15:12:16 PST
<rdar://problem/72199236>
Comment 16 Keith Miller 2020-12-14 12:47:34 PST
Comment on attachment 415933 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=415933&action=review

> Source/JavaScriptCore/API/JSLockRefPrivate.h:40
> +JS_EXPORT void JSLock(JSContextRef ctx);

Shouldn't this take a JSContextGroupRef? We lock the entire VM which corresponds to a group. With this API I'd expect to be able to have two different JSContextRefs from the same group locked and running on different threads at the same time. If we ever add a concurrent JS feature in the future we can add this version back, especially since people could use this API today as a thread exclusion. So making this API work across threads could be a compatibly issue in the future.