Re: How to name our JSContext getter(s): let the bikeshed begin
On 5/30/16 12:55 PM, Kyle Huey wrote: Segregating the thread-specific and thread-agnostic parts into separate classes seems like a good idea. Just to be clear, that's pretty much all internal stuff. From the perspective of an API consumer (Gecko), there is really only the thread-specific thing in spidermonkey API. Put another way, as far as I can tell, SpiderMonkey will continue to have an API where there is some thing (name not clear yet, but currently it's called JSRuntime) that maps to some specific thread of the embedding. That thread is the only embedder thread allowed to touch anything to do with that JSRuntime. SpiderMonkey will internally create various helper threads (e.g. for JIT compilation) which may touch some things that may or may not live on JSRuntime; this is the part still being decided. But from the point of view of the embedding, all that stuff doesn't even exist. Given that it only makes sense to use a thread-specific object on that thread, and it only makes sense to get the thread-agnostic object here *from TLS* on one thread, I don't think we need any "thread" naming. OK. So what do you propose we name things? -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: How to name our JSContext getter(s): let the bikeshed begin
On Sun, May 29, 2016 at 6:21 PM, Boris Zbarsky wrote: > On 5/29/16 6:17 PM, Kyle Huey wrote: >> >> Do we really need the ForThread part? > > > I wanted to make it clear that we're getting something that's OK to use > without synchronization, but maybe that's redundant and we can just have a > dom::GetJSContext() or something. dom::JSContext() would have ambiguity > issues, of course. I don't have super-strong opinions here. > >> Is the long term plan to merge >> JSRuntime and JSContext, or are they going to remain distinct >> indefinitely? > > > Unclear. See discussion the SpiderMonkey folks are having starting at > https://bugzilla.mozilla.org/show_bug.cgi?id=650361#c27 Segregating the thread-specific and thread-agnostic parts into separate classes seems like a good idea. Given that it only makes sense to use a thread-specific object on that thread, and it only makes sense to get the thread-agnostic object here *from TLS* on one thread, I don't think we need any "thread" naming. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: How to name our JSContext getter(s): let the bikeshed begin
On 05/30/2016 05:46 AM, Boris Zbarsky wrote: On 5/29/16 6:21 PM, Boris Zbarsky wrote: I wanted to make it clear that we're getting something that's OK to use without synchronization, but maybe that's redundant and we can just have a dom::GetJSContext() or something. dom::JSContext() would have ambiguity issues, of course. I don't have super-strong opinions here. Another thought that just occurred to me is ThreadCx(). -Boris I like that. I'd prefer to have it as a static method in some class (to ease readability and searchability of the code), but I admit dom::ThreadCx(); (== mozilla::dom::ThreadCx()) looks pretty nice. So I guess method in dom namespace is fine too. I wonder if we should have some single place (.h/.cpp) where we put all global methods in dom namespace. -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: How to name our JSContext getter(s): let the bikeshed begin
On 5/29/16 6:21 PM, Boris Zbarsky wrote: I wanted to make it clear that we're getting something that's OK to use without synchronization, but maybe that's redundant and we can just have a dom::GetJSContext() or something. dom::JSContext() would have ambiguity issues, of course. I don't have super-strong opinions here. Another thought that just occurred to me is ThreadCx(). -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: How to name our JSContext getter(s): let the bikeshed begin
On 5/29/16 6:17 PM, Kyle Huey wrote: Do we really need the ForThread part? I wanted to make it clear that we're getting something that's OK to use without synchronization, but maybe that's redundant and we can just have a dom::GetJSContext() or something. dom::JSContext() would have ambiguity issues, of course. I don't have super-strong opinions here. Is the long term plan to merge JSRuntime and JSContext, or are they going to remain distinct indefinitely? Unclear. See discussion the SpiderMonkey folks are having starting at https://bugzilla.mozilla.org/show_bug.cgi?id=650361#c27 -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: How to name our JSContext getter(s): let the bikeshed begin
Do we really need the ForThread part? Is the long term plan to merge JSRuntime and JSContext, or are they going to remain distinct indefinitely? - Kyle On Sun, May 29, 2016 at 6:10 PM, Boris Zbarsky wrote: > We currently have the following functions in nsContentUtils for getting > various JSContexts: > > GetSafeJSContext() > GetDefaultJSContextForThread() > RootingCx() > RootingCxForThread() > > We also have workers::GetCurrentThreadJSContext() for use on workers only. > > Now that we're about to move to only having one JSContext per thread (see > bug 1276276) I think we should do some consolidating and renaming here. In > particular, we should stash the unique JSContext for the thread in the > CycleCollectedJSRuntime and have only one accessor to get it, which goes > through CycleCollectedJSRuntime::Get(). This does mean a TLS lookup in some > cases in which right now we just do a pointer-chase, but has the benefit of > simplicity. And if we ever add non-worker threads with DOM stuff on them > (which we're talking about anyway), we'd want this no matter what. > > My proposal is that we name this accessor something like > JSContextForThread(). We can put this in nsContentUtils, in the > "mozilla::dom' namespace, or the "xpc" namespace... I don't have a strong > preference here. > > If we _really_ want to we can keep a RootingCxForThread() around which > returns exactly the same thing as JSContextForThread() but makes it clear > that we plan to use it for rooting only. I think we should nix RootingCx(). > > Thoughts? > > -Boris > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
How to name our JSContext getter(s): let the bikeshed begin
We currently have the following functions in nsContentUtils for getting various JSContexts: GetSafeJSContext() GetDefaultJSContextForThread() RootingCx() RootingCxForThread() We also have workers::GetCurrentThreadJSContext() for use on workers only. Now that we're about to move to only having one JSContext per thread (see bug 1276276) I think we should do some consolidating and renaming here. In particular, we should stash the unique JSContext for the thread in the CycleCollectedJSRuntime and have only one accessor to get it, which goes through CycleCollectedJSRuntime::Get(). This does mean a TLS lookup in some cases in which right now we just do a pointer-chase, but has the benefit of simplicity. And if we ever add non-worker threads with DOM stuff on them (which we're talking about anyway), we'd want this no matter what. My proposal is that we name this accessor something like JSContextForThread(). We can put this in nsContentUtils, in the "mozilla::dom' namespace, or the "xpc" namespace... I don't have a strong preference here. If we _really_ want to we can keep a RootingCxForThread() around which returns exactly the same thing as JSContextForThread() but makes it clear that we plan to use it for rooting only. I think we should nix RootingCx(). Thoughts? -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
