| Summary: | Add a JSC API to allow acquiring the JSLock | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||
| Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, ews-watchlist, fpizlo, ggaren, jbedard, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Tadeu Zagallo
2020-12-08 18:41:04 PST
Created attachment 415700 [details]
Patch
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 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 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 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. Created attachment 415793 [details]
Patch
> 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. > 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.
(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 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 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. Created attachment 415933 [details]
Patch for landing
> > 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. Committed r270659: <https://trac.webkit.org/changeset/270659> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415933 [details]. 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. |