Closed
Bug 1361900
Opened 7 years ago
Closed 7 years ago
Use script precompiler in content processes
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 2 open bugs)
Details
Attachments
(10 files)
59 bytes,
text/x-review-board-request
|
JamesCheng
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
gkrizsanits
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
This is a follow-up to bug 1359653 to enable that code in content processes, rather than just the parent process.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8864349 -
Flags: review?(gkrizsanits)
Attachment #8864351 -
Flags: review?(gkrizsanits)
Attachment #8864353 -
Flags: review?(gkrizsanits)
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8864346 [details] Bug 1361900: Part 1 - Make CDMProxy.h compatible with mozilla::Result. https://reviewboard.mozilla.org/r/135996/#review139064 r+ with nits fixing. ::: dom/media/eme/CDMProxy.h:31 (Diff revision 1) > NoKeyErr = 2, > AbortedErr = 3, > }; > +} > + > +using eme::DecryptStatus; Avoid using namespace in header file. Maybe you can use it once Cpp wants to use it. ::: dom/media/gmp/ChromiumCDMProxy.cpp:15 (Diff revision 1) > #include "nsPrintfCString.h" > #include "GMPService.h" > > namespace mozilla { > > +using namespace eme; Why using namespace here? ::: dom/media/gmp/DecryptJob.cpp:44 (Diff revision 1) > - if (aResult == Ok) { > + if (aResult == eme::Ok) { > UniquePtr<MediaRawDataWriter> writer(mSample->CreateWriter()); > PodCopy(writer->Data(), > aDecryptedData.Elements(), > std::min<size_t>(aDecryptedData.Length(), mSample->Size())); > } else if (aResult == NoKeyErr) { For consistency, I would prefer to specify the namespace for all the reset enums. ::: dom/media/gmp/GMPDecryptorParent.cpp:405 (Diff revision 1) > > DecryptStatus > ToDecryptStatus(GMPErr aError) > { > switch (aError) { > - case GMPNoErr: return Ok; > + case GMPNoErr: return eme::Ok; I'm not sure why you only specify the eme only on Ok(since it is too common that makes you compile failure?). I suggest not using namespace eme but writing eme::Ok, eme::NoKeyErr, eme::AbortedErr instead for consistency. ::: dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp:27 (Diff revision 1) > #include "nsServiceManagerUtils.h" > #include "DecryptThroughputLimit.h" > > namespace mozilla { > > +using namespace eme; IIUC, don't need to using since you specify the eme:: below?
Attachment #8864346 -
Flags: review?(jacheng) → review+
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8864349 [details] Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. https://reviewboard.mozilla.org/r/136002/#review139966 Sorry for the delay it took me a while to read them through. I have a bunch of questions so I cannot yet r+ all the patches yet but it looks really nice so far. ::: dom/base/nsFrameMessageManager.cpp:1611 (Diff revision 1) > if (XRE_IsParentProcess()) { > - script = ScriptPreloader::GetSingleton().GetCachedScript(cx, url); > + script = ScriptPreloader::GetChildSingleton().GetCachedScript(cx, url); > } I don't think you're caching scripts that are loaded in the child process but the ones we load via the mm, no? Shouldn't the name reflect that instead? The child is somewhat inaccurate but I might be missing something. ::: js/xpconnect/loader/ScriptPreloader.cpp:277 (Diff revision 1) > NS_TRY(mProfD->Clone(getter_AddRefs(cacheFile))); > > NS_TRY(cacheFile->AppendNative(NS_LITERAL_CSTRING("startupCache"))); > Unused << cacheFile->Create(nsIFile::DIRECTORY_TYPE, 0777); > > - NS_TRY(cacheFile->AppendNative(nsDependentCString(leafName))); > + NS_TRY(cacheFile->Append(mBaseName + suffix)); Thanks for working on a macro for this. Could you make sure once you've done with the final version (I've seen your thread on the mailing list about it) that you add comment to it with: reason + example for usage. This is unrelated to this patch ofc. ::: js/xpconnect/loader/ScriptPreloader.cpp:424 (Diff revision 1) > void > ScriptPreloader::PrepareCacheWrite() > { > MOZ_ASSERT(NS_IsMainThread()); > > + auto cleanup = MakeScopeExit([&] () { Nit: any reason this is [&] instead of []?
Attachment #8864349 -
Flags: review?(gkrizsanits) → review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review140012 I guess what I'm missing the most from this patch is a detailed description of what does the caching of frame/process scripts really do. What I gathered from the code so far is that: - the first process builds up a cache and sends it back to the parent, who writes it to a file and shares this file with the subsequent content processes via an fd - until the first process finishing its initialization, the other processes will not benefit from the cache - we cache scripts based on process type - frame / process scripts that are unique to one process / tab, and are not present in the first content process at its startup will not be shared - it's only inited once, anything that comes later will not be shared / cached, does it make sense to do this at each startup? Shouldn't we just reause the previous cache and reset it during browser update? Or do we already do that? (sorry I might get lost in the patches and missed this part) ::: js/xpconnect/loader/ScriptCacheActors.h:26 (Diff revision 1) > +public: > + explicit ScriptCacheParent(bool wantCacheData) > + : mWantCacheData(wantCacheData) > + {} > + > +private: > + bool mWantCacheData; > + > +protected: > + virtual IPCResult Recv__delete__(nsTArray<ScriptData>&& scripts) override; > + > + virtual void ActorDestroy(ActorDestroyReason aWhy) override; Nit: In general I'd prefer either public-protected-private or private-protected-public orders. But I could not find any guidelines in our docs and our code does not seem to be doing a great job enforcing any order. So feel free to decide if you want to change it or not. ::: js/xpconnect/loader/ScriptCacheActors.cpp:34 (Diff revision 1) > +} > + > +// Finalize the script cache for the content process, and send back data about > +// any scripts executed up to this point. > +void > +ScriptCacheChild::Finalize(LinkedList<ScriptPreloader::CachedScript>& scripts) Finalize is a bit misleading... this method does not have anything with the GC right? Can we just name it differently? ::: js/xpconnect/loader/ScriptCacheActors.cpp:44 (Diff revision 1) > + > + AutoSafeJSAPI jsapi; > + > + nsTArray<ScriptData> dataArray; > + for (auto script : scripts) { > + if (!script->mSize && !script->XDREncode(jsapi.cx())) { What can be the reason for XDREncode returning false here? Should we warn / assert in debug mode? ::: js/xpconnect/loader/ScriptPreloader-inl.h:25 (Diff revision 1) > +using mozilla::dom::AutoJSAPI; > + > +struct MOZ_RAII AutoSafeJSAPI : public AutoJSAPI > +{ > + AutoSafeJSAPI() { Init(); } > +}; > + I think this should go into a more generic section like ScriptSettings.h. Also I wouldn't call it Safe but something that reflects that it does not enter any compartment. It's totally not safe to run any script with this for example, since that will only fail. ::: js/xpconnect/loader/ScriptPreloader.cpp:129 (Diff revision 1) > + // That process sends back its script data before it executes any > + // untrusted code, and then we never accept further script data for that > + // type of process for the rest of the session. > + // > + // The script data from each process type is merged with the data from the > + // parent process's frame scripts, and shared between all content process Nit: frame and process scripts ::: js/xpconnect/loader/ScriptPreloader.cpp:133 (Diff revision 1) > + // The script data from each process type is merged with the data from the > + // parent process's frame scripts, and shared between all content process > + // types in the next session. > + auto processType = GetProcessType(parent->GetRemoteType()); > + bool wantScriptData = !cache.mInitializedProcesses.contains(processType); > + cache.mInitializedProcesses += processType; What if the first process crashes or gets shut down for some weird reason? ::: js/xpconnect/loader/ScriptPreloader.cpp:135 (Diff revision 1) > + auto fd = cache.mCacheData.cloneFileDescriptor(); > + if (fd.IsValid()) { > + Unused << parent->SendPScriptCacheConstructor(fd, wantScriptData); > + } else { > + Unused << parent->SendPScriptCacheConstructor(NS_ERROR_FILE_NOT_FOUND, wantScriptData); > + } > +} I guess this part is built on top of some patches those have not landed yet so I cannot double check it. But what I would love to avoid is that we send a file descriptor to the child that has write access. I assume on the parent side we have rw access so if cloneFileDescriptor returns a read only descriptor then its name is a bit missleading... ::: js/xpconnect/loader/ScriptPreloader.cpp:196 (Diff revision 1) > + // In the child process, we need to freeze the script cache before any > + // untrusted code has been executed. The insertion of the first DOM > + // document element may sometimes be earlier than is ideal, but at > + // least it should always be safe. > + obs->AddObserver(this, DOC_ELEM_INSERTED_TOPIC, false); This might change in the very near future but right now add-ons can load process scripts which can potentially run sooner than any document would exists, especially for the preallocated process. ::: js/xpconnect/loader/ScriptPreloader.cpp:610 (Diff revision 1) > size_t offset = 0; > for (auto script : mSavedScripts) { > script->mOffset = offset; > script->Code(buf); > > offset += script->mSize; What guarantees that we don't overflow here on a 32bit system?
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8864353 [details] Bug 1361900: Part 8 - Use the script preloader in content processes in the frame script loader. https://reviewboard.mozilla.org/r/136010/#review140066
Attachment #8864353 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864349 [details] Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. https://reviewboard.mozilla.org/r/136002/#review139966 > I don't think you're caching scripts that are loaded in the child process but the ones we load via the mm, no? Shouldn't the name reflect that instead? The child is somewhat inaccurate but I might be missing something. Well, I was thinking of it in the same way that we have in-process content children in the parent process. The child singleton is for caching scripts that will (probably) be used in the child, even if we're running in the parent. > Nit: any reason this is [&] instead of []? We need to capture at least [this], but [&] is basically the standard capture for ScopeExit closures, since they're always destroyed before the end of the current scope (by definition).
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review140012 That is a mostly accurate summary, except for the fact that content processes get the cache data from the *last* session. Essentially: - Every startup, we open the cache file from the last session, move it to a new location, and begin pre-loading the scripts that are stored in it. There is a separate cache file for parent and content processes, but the parent process opens both the parent and content cache files. - Once startup is complete, we write a new cache file for the next session, containing only the scripts that were used during early startup, so we don't waste pre-loading scripts that may not be needed. - For content processes, opening and writing the cache file is handled in the parent process. The first content process of each type sends back the data for scripts that were loaded in early startup, and the parent merges them and writes them to a cache file. - Currently, content processes only benefit from the cache data written during the *previous* session. Ideally, new content processes should probably use the cache data written during this session if there was no previous cache file, but I'd rather do that as a follow-up. > Finalize is a bit misleading... this method does not have anything with the GC right? Can we just name it differently? I guess it might be confusing if you're expecting it to be a GC finalizer, but it is a finalizer in the sense that it's what destroys the object. > What can be the reason for XDREncode returning false here? Should we warn / assert in debug mode? In practice, probably just EOM. A warning might make sense. > I think this should go into a more generic section like ScriptSettings.h. Also I wouldn't call it Safe but something that reflects that it does not enter any compartment. It's totally not safe to run any script with this for example, since that will only fail. I named it based on the docs for the Init() method :) // This uses the SafeJSContext (or worker equivalent), and enters a null // compartment, so that the consumer is forced to select a compartment to // enter before manipulating objects. > What if the first process crashes or gets shut down for some weird reason? Then we don't get any cache data for the session. I thought about trying to handle that case, but it didn't seem worth it. The only side-effect is a slight performance regression for the next session, and only if the content process crashes *very* early, so I'd rather just document it than trying to handle it. > I guess this part is built on top of some patches those have not landed yet so I cannot double check it. But what I would love to avoid is that we send a file descriptor to the child that has write access. I assume on the parent side we have rw access so if cloneFileDescriptor returns a read only descriptor then its name is a bit missleading... The fd is readonly. The cache file for the current session is essentially immutable, and the next session's cache gets written out to a different file. The patches for this are in bug 1359653, but they've also landed at this point, and in this bug. > This might change in the very near future but right now add-ons can load process scripts which can potentially run sooner than any document would exists, especially for the preallocated process. Well, I was considering add-on code to be trusted code for these purposes. > What guarantees that we don't overflow here on a 32bit system? All of the scripts are stored in memory at this point, so if we have enough address space to map the memory for them, they're guaranteed to fit into a size_t.
Comment 17•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #15) > Well, I was thinking of it in the same way that we have in-process content > children in the parent process. The child singleton is for caching scripts > that will (probably) be used in the child, even if we're running in the > parent. It's more of a personal preference that anything else, your version makes sense too and fits in our existing terminology just fine, so feel free to keep it if you prefer this version. (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #16) > That is a mostly accurate summary, except for the fact that content > processes get the cache data from the *last* session. Essentially: Thanks for the detailed walkthrough! If you could add something like this as a comment in one of the headers, that would be awesome. > - Currently, content processes only benefit from the cache data written > during the *previous* session. Ideally, new content processes should > probably use the cache data written during this session if there was no > previous cache file, but I'd rather do that as a follow-up. I wouldn't worry about the no previous cache version case. Just one more question, is there a time where we clear the cache? Like after ff update? Or for example if I change a script file while working on something and restart the browser will it detect the change or will I have to restart it twice now? I would assume this is already solved for the regular cache and we just reset this one at the same time but wanted to double check it. > I guess it might be confusing if you're expecting it to be a GC finalizer, > but it is a finalizer in the sense that it's what destroys the object. It does two things, sends the data up the parent to store it AND destroys. Arguably the more important operation is the former, FlushAndDestroy, StoreAndFinalize, UpdateAndSomething... It's not unexpected that a cache in its finalize method saves the data though... so if you feel strongly about the bare finalizer name I'm not going to argue against it any more. > > > What can be the reason for XDREncode returning false here? Should we warn / assert in debug mode? > > In practice, probably just EOM. A warning might make sense. Might help as a sanity check... > > > I think this should go into a more generic section like ScriptSettings.h. Also I wouldn't call it Safe but something that reflects that it does not enter any compartment. It's totally not safe to run any script with this for example, since that will only fail. > > I named it based on the docs for the Init() method :) > > // This uses the SafeJSContext (or worker equivalent), and enters a null > // compartment, so that the consumer is forced to select a compartment to > // enter before manipulating objects. > And safe does makes sense there, I mean the SafeJSContext. But for the JSAPI, which is often used to run scripts, it makes less sense to me. The next init method in the same file uses the SafeJSContext as well but also enters the compartment of the global it takes as the argument, after which JS operations make sense. Anyway, the best would be to check this with either Boris or Bobby and place this helper in that file (I'm fine with whatever name they feel comfortable with). > > What if the first process crashes or gets shut down for some weird reason? > > Then we don't get any cache data for the session. I thought about trying to > handle that case, but it didn't seem worth it. The only side-effect is a > slight performance regression for the next session, and only if the content > process crashes *very* early, so I'd rather just document it than trying to > handle it. > Yeah, a brief comment about it is perfect. > The fd is readonly. The cache file for the current session is essentially > immutable, and the next session's cache gets written out to a different > file. The patches for this are in bug 1359653, but they've also landed at > this point, and in this bug. Thanks, sounds perfect. > > > This might change in the very near future but right now add-ons can load process scripts which can potentially run sooner than any document would exists, especially for the preallocated process. > > Well, I was considering add-on code to be trusted code for these purposes. Right, so this is about preventing any web content compromising the content process, to reach the parent or other content processes. Makes sense then. > > > What guarantees that we don't overflow here on a 32bit system? > > All of the scripts are stored in memory at this point, so if we have enough > address space to map the memory for them, they're guaranteed to fit into a > size_t. Sorry, I had a silly chain of thoughts here and over-complicated something in my mind (AKA I was stupid). I'll run through the patch real quickly but seems like an r+ from here.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review140188 I've added my comments to bugzilla r=me with those.
Attachment #8864351 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17) > (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment > #16) > > That is a mostly accurate summary, except for the fact that content > > processes get the cache data from the *last* session. Essentially: > > Thanks for the detailed walkthrough! If you could add something like this as > a comment in one of the headers, that would be awesome. Will do. > I wouldn't worry about the no previous cache version case. Just one more > question, is there a time where we clear the cache? Like after ff update? Or > for example if I change a script file while working on something and restart > the browser will it detect the change or will I have to restart it twice > now? I would assume this is already solved for the regular cache and we just > reset this one at the same time but wanted to double check it. Yes. We currently clear the cache any time the normal startup cache is flushed, so: - When the app changes. - When the -purgeCaches flag is passed. - When someone sends a "startupcache-invalidate" observer notification (which happens any time an add-on is installed or updated) > > I guess it might be confusing if you're expecting it to be a GC finalizer, > > but it is a finalizer in the sense that it's what destroys the object. > > It does two things, sends the data up the parent to store it AND destroys. > Arguably the more important operation is the former, FlushAndDestroy, > StoreAndFinalize, UpdateAndSomething... It's not unexpected that a cache in > its finalize method saves the data though... so if you feel strongly about > the bare finalizer name I'm not going to argue against it any more. OK. I guess a more descriptive name wouldn't hurt.
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8864352 [details] Bug 1361900: Part 7 - Use the script preloader in content processes in subscript and component loaders. https://reviewboard.mozilla.org/r/136008/#review140710 ::: js/xpconnect/loader/mozJSComponentLoader.cpp:700 (Diff revision 1) > - rv = ReadCachedFunction(cache, cachePath, cx, mSystemPrincipal, > + rv = ReadCachedFunction(cache, cachePath, cx, mSystemPrincipal, > - function.address()); > + function.address()); > - } > + } > > - if (NS_SUCCEEDED(rv)) { > + if (NS_SUCCEEDED(rv)) { > - LOG(("Successfully loaded %s from startupcache\n", nativePath.get())); > + LOG(("Successfully loaded %s from startupcache\n", nativePath.get())); This LOG message isn't correct any more, is it? Don't we also hit NS_SUCCEEDED if the script isn't in the preloader and we don't have a cache? (Also maybe something is weird with the mReuseLoaderGlobal case but whatever.) Should the code above reset rv to a FAIL value? Maybe this check should be "script || function" instead? ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp (Diff revision 1) > > if (startupCache) { > JSAutoCompartment ac(cx, script); > WriteCachedScript(StartupCache::GetSingleton(), > cachePath, cx, principal, script); > ->>>>>>> histedit I'm not sure what this is.
Attachment #8864352 -
Flags: review?(continuation) → review-
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8864347 [details] Bug 1361900: Part 2 - Add process types field to cached script data. https://reviewboard.mozilla.org/r/135998/#review140830 Looks good, just some minor changes / clarifications requested. ::: js/xpconnect/loader/ScriptPreloader-inl.h:62 (Diff revision 1) > } > > void > + codeUint8(const uint8_t& val) > + { > + *write(1) = val; nit: sizeof please ::: js/xpconnect/loader/ScriptPreloader-inl.h:67 (Diff revision 1) > + *write(1) = val; > + } > + > + template<typename T> > + void > + codeUint8(EnumSet<T>& val) Perhaps this should be called `codeEnumSet` or `codeProcessType` and it could just take a `EnumSet<ProcessType>` as a param. We should also just add a static_assert below that `sizeof(T) == sizeof(uint8_t)`, you could then get rid of the CheckedInt. ::: js/xpconnect/loader/ScriptPreloader-inl.h:69 (Diff revision 1) > + > + template<typename T> > + void > + codeUint8(EnumSet<T>& val) > + { > + *write(1) = CheckedUint8(val.serialize()).value(); Can you maybe split this into two lines w/ a comment? It took me a minute to work out what was goin on: - EnumSet.serialize returns a uint32_t, we want a uint8_t so we use CheckUint8 to safely convert - We then want to write out the uint8_t Also can you use sizeof? ::: js/xpconnect/loader/ScriptPreloader-inl.h:125 (Diff revision 1) > } > > bool > + codeUint8(uint8_t& val) > + { > + if (checkCapacity(1)) { nit: sizeof please ::: js/xpconnect/loader/ScriptPreloader-inl.h:135 (Diff revision 1) > + > + template<typename T> > + bool > + codeUint8(EnumSet<T>& val) > + { > + if (checkCapacity(1)) { nit: sizeof ::: js/xpconnect/loader/ScriptPreloader.h:34 (Diff revision 1) > namespace mozilla { > namespace loader { > class InputBuffer; > + > + enum class ProcessType : uint8_t { > + Default, What does default mean? Can you add some comments here. I'm guessing web means a content process that is rendering a website. Extension is...a content process for a web extension? Do we care about GPU? There'se probably others.... ::: js/xpconnect/loader/ScriptPreloader.h:200 (Diff revision 1) > // The off-thread decode token for a completed off-thread decode, which > // has not yet been finalized on the main thread. > void* mToken = nullptr; > > + // The set of processes in which this script has been used. > + EnumSet<ProcessType> mProcesses{}; Perhaps `mProcessTypes` or `mProcessTypeSet` would be more clear here. ::: js/xpconnect/loader/ScriptPreloader.cpp:99 (Diff revision 1) > +ScriptPreloader::GetProcessType(const nsAString& remoteType) > +{ > + if (remoteType.EqualsLiteral(EXTENSION_REMOTE_TYPE)) { > + return ProcessType::Extension; > + } > + return ProcessType::Web; Do we want to differentiate chrome from content? ::: js/xpconnect/loader/ScriptPreloader.cpp:137 (Diff revision 1) > > ScriptPreloader::ScriptPreloader() > : mMonitor("[ScriptPreloader.mMonitor]") > , mSaveMonitor("[ScriptPreloader.mSaveMonitor]") > { > + if (XRE_IsParentProcess()) { Can you just encapsulate this logic in `GetProcessType`? ::: js/xpconnect/loader/ScriptPreloader.cpp:258 (Diff revision 1) > NS_TRY(cacheFile->AppendNative(nsDependentCString(leafName))); > > return Move(cacheFile); > } > > -static const uint8_t MAGIC[] = "mozXDRcache"; > +static const uint8_t MAGIC[] = "mozXDRcachev001"; This is all well and good for a new build reading an old cache, but not for an old build reading a new cache. `mozXDRcach1` or something would work. I don't feel super strongly about this since it'll only affect nightly users (I think). Also hopefully our follow up checks reading in the other params will trip up as well. ::: js/xpconnect/loader/ScriptPreloader.cpp:370 (Diff revision 1) > auto start = TimeStamp::Now(); > LOG(Info, "Off-thread decoding scripts...\n"); > > JS::CompileOptions options(cx, JSVERSION_LATEST); > for (auto script : mRestoredScripts) { > - if (script->mSize > MIN_OFFTHREAD_SIZE && > + if (script->mProcesses.contains(CurrentProcessType()) && Can you add a comment saying something like: "Only decode scripts that have been used in this process type". `mProcesses` is kind of a generic name (it seems like it's a list of processes) ::: js/xpconnect/loader/moz.build:46 (Diff revision 1) > '../src', > '../wrappers', > '/dom/base', > ] > > +include('/ipc/chromium/chromium-config.mozbuild') Whats up here? Is that for `dom::ContentChild`?
Attachment #8864347 -
Flags: review?(erahm) → review-
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8864348 [details] Bug 1361900: Part 3 - Add a helper script to decode and inspect script cache data. https://reviewboard.mozilla.org/r/136000/#review140842 Some small comments, but given this is just a helper r=me. ::: js/xpconnect/loader/script_cache.py:5 (Diff revision 1) > +#!/usr/bin/env python > +import io > +import struct > +import sys > + Can you add a comment here with usage and explanation of what this does (or print out a usage message if no files are passed in) ::: js/xpconnect/loader/script_cache.py:6 (Diff revision 1) > +#!/usr/bin/env python > +import io > +import struct > +import sys > + > +MAGIC = b'mozXDRcachev001\0' Ugh we have to keep this in sync? It seems like we'd want to handle multiple versions in this script if we ever upgrade. So maybe we just say the magic header is len(mozXDRcachev001\0), read that in and later compare it against one's we suppport.
Attachment #8864348 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864352 [details] Bug 1361900: Part 7 - Use the script preloader in content processes in subscript and component loaders. https://reviewboard.mozilla.org/r/136008/#review140710 > This LOG message isn't correct any more, is it? Don't we also hit NS_SUCCEEDED if the script isn't in the preloader and we don't have a cache? (Also maybe something is weird with the mReuseLoaderGlobal case but whatever.) Should the code above reset rv to a FAIL value? Maybe this check should be "script || function" instead? Yeah, I think checking `script || function` is probably a good idea. > I'm not sure what this is. It got added in a rebase, but I didn't realize it happened before I pushed to review or I would have updated.
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864347 [details] Bug 1361900: Part 2 - Add process types field to cached script data. https://reviewboard.mozilla.org/r/135998/#review140830 > Perhaps this should be called `codeEnumSet` or `codeProcessType` and it could just take a `EnumSet<ProcessType>` as a param. We should also just add a static_assert below that `sizeof(T) == sizeof(uint8_t)`, you could then get rid of the CheckedInt. I'm more concerned about the storage size than the fact that it's an EnumSet. I was originally just using `codeUint8`, but it required funky code on the caller side to handle both the read and the write cases correctly. I'd like to try to keep this code as generic as possible so we can extract it and use it elsewhere, if it seems like it would be useful, so I'd like to avoid tying it to specific types. It would be nice to at some point unify this code with the XDR transcoding code. > Can you maybe split this into two lines w/ a comment? It took me a minute to work out what was goin on: > - EnumSet.serialize returns a uint32_t, we want a uint8_t so we use CheckUint8 to safely convert > - We then want to write out the uint8_t > > Also can you use sizeof? Yeah, makes sense. > What does default mean? Can you add some comments here. I'm guessing web means a content process that is rendering a website. Extension is...a content process for a web extension? Do we care about GPU? There'se probably others.... It's what the parent process type is called internally. Renaming it "Parent" would probably be fine here, though. We don't care about GPU or others, because we never load scripts into them. > Do we want to differentiate chrome from content? This is only used for content processes. It should probably be called "GetContentProcessType", or something. > Can you just encapsulate this logic in `GetProcessType`? That function only applies to content processes, so I should probably rename it. > This is all well and good for a new build reading an old cache, but not for an old build reading a new cache. `mozXDRcach1` or something would work. > > I don't feel super strongly about this since it'll only affect nightly users (I think). Also hopefully our follow up checks reading in the other params will trip up as well. The null terminator is part of the magic number, so this works both ways. > Whats up here? Is that for `dom::ContentChild`? Yeah, we can't include ContentChild.h without it, but it's also needed for the IPDL stuff I added in later patches.
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864348 [details] Bug 1361900: Part 3 - Add a helper script to decode and inspect script cache data. https://reviewboard.mozilla.org/r/136000/#review140842 > Ugh we have to keep this in sync? It seems like we'd want to handle multiple versions in this script if we ever upgrade. So maybe we just say the magic header is len(mozXDRcachev001\0), read that in and later compare it against one's we suppport. I'm only concerned about supporting a single version at once, since we throw away the old cache file whenever we upgrade, anyway. The magic number check is just to make sure we don't somehow wind up trying to parse the wrong version without noticing.
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8864349 [details] Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. https://reviewboard.mozilla.org/r/136002/#review140844 Just looking for some clarification / comments here. I'm reasonably sure we don't need a strong ref to the cache. ::: js/xpconnect/loader/ScriptPreloader.h:112 (Diff revision 1) > { > public: > CachedScript(CachedScript&&) = default; > > - CachedScript(const nsCString& url, const nsCString& cachePath, JSScript* script) > - : mURL(url) > + CachedScript(ScriptPreloader* cache, const nsCString& url, const nsCString& cachePath, JSScript* script) > + : mCache(cache) Should this be a ref instead? I think it's expected to be non-null, we don't own it, and we don't plan on changing the value of `mCache`. Edit: oh we strong ref it, hmmm. Is that necessary? Is the CachedScript ever expected to outlive the cache? ::: js/xpconnect/loader/ScriptPreloader.cpp:459 (Diff revision 1) > > for (CachedScript* next = mSavedScripts.getFirst(); next; ) { > CachedScript* script = next; > next = script->getNext(); > > - if (!script->mSize && !script->XDREncode(jsapi.cx())) { > + // Don't write any scripts that are also in the child cache. Why not? If it's something we load in the parent process then we probably want to preload it on the next run. Edit: oh okay, maybe elaborate on the "since we'll load it from the child cache instead" ::: js/xpconnect/loader/ScriptPreloader.cpp:638 (Diff revision 1) > JSScript* > ScriptPreloader::GetCachedScript(JSContext* cx, const nsCString& path) > { > + // If a script is used by both the parent and the child, it's stored only > + // in the child cache. > + if (mChildCache) { Is this more likely than it being in the parent cache? I just want to see if the checks should be swapped.
Attachment #8864349 -
Flags: review?(erahm) → review-
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8864350 [details] Bug 1361900: Part 5 - Add support for IPC FileDescriptors to AutoMemMap. https://reviewboard.mozilla.org/r/136004/#review140856 This seems okay, you might want to tag someone more familiar with IPC + file descriptors to make sure it's sane. Maybe jld? ::: js/xpconnect/loader/AutoMemMap.cpp:13 (Diff revision 1) > #include "ScriptPreloader-inl.h" > > #include "mozilla/Unused.h" > #include "nsIFile.h" > > +#include <private/pprio.h> Can you add a `// why this is needed`, it looks sketchy but I get it cargo culted.
Attachment #8864350 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864350 [details] Bug 1361900: Part 5 - Add support for IPC FileDescriptors to AutoMemMap. https://reviewboard.mozilla.org/r/136004/#review140856 > Can you add a `// why this is needed`, it looks sketchy but I get it cargo culted. I don't cargo cult :) It's necessary for `PR_ImportFile`, since there isn't a better way to turn an OS file handle into a NSPR file handle, and we need the latter for `PR_MemMap`.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8864352 [details] Bug 1361900: Part 7 - Use the script preloader in content processes in subscript and component loaders. https://reviewboard.mozilla.org/r/136008/#review140910
Attachment #8864352 -
Flags: review?(continuation) → review+
Updated•7 years ago
|
Blocks: webext-perf
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8864347 [details] Bug 1361900: Part 2 - Add process types field to cached script data. https://reviewboard.mozilla.org/r/135998/#review141724 ::: js/xpconnect/loader/ScriptPreloader.cpp:475 (Diff revisions 1 - 2) > // an offset from the start of the block, as specified above. > Result<Ok, nsresult> > ScriptPreloader::WriteCache() > { > MOZ_ASSERT(!NS_IsMainThread()); > Might be worth asserting the monitor is locked here. ::: js/xpconnect/loader/ScriptPreloader.cpp:746 (Diff revisions 1 - 2) > } > > return mScript; > } > > + Maybe<JSAutoCompartment> ac; I think you need to get an r+ for this from someone who understands what it does. Is it switching compartments for the compilation?
Attachment #8864347 -
Flags: review?(erahm) → review+
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8864349 [details] Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. https://reviewboard.mozilla.org/r/136002/#review141728
Attachment #8864349 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864347 [details] Bug 1361900: Part 2 - Add process types field to cached script data. https://reviewboard.mozilla.org/r/135998/#review141724 > I think you need to get an r+ for this from someone who understands what it does. Is it switching compartments for the compilation? This wasn't part of this patch. mozreview is just lying to you. But essentially, yes, it's switching compartments to finish decoding.
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review141746 Looks good so far, I just have a bunch of questions so I'm going to r- for now. ::: js/xpconnect/loader/PScriptCache.ipdl:19 (Diff revision 2) > + > +struct ScriptData { > + nsCString url; > + nsCString cachePath; > + TimeStamp loadTime; > + uint8_t[] xdrData; I'm assuming ipdl fields are optional? Or I guess we can do a zero-length array... Might be worth documenting that this isn't always used. ::: js/xpconnect/loader/ScriptCacheActors.cpp:27 (Diff revision 2) > + Unused << cache.InitCache(cacheFile, this); > + > + if (!wantCacheData) { > + // If the parent process isn't expecting any cache data from us, we're > + // done. > + Send__delete__(this, AutoTArray<ScriptData, 0>()); Not sure if I remember/understand this properly, but in the `wantCacheData == true` case is it possible the IPC message will timeout? ::: js/xpconnect/loader/ScriptCacheActors.cpp:36 (Diff revision 2) > +// Finalize the script cache for the content process, and send back data about > +// any scripts executed up to this point. > +void > +ScriptCacheChild::Finalize(LinkedList<ScriptPreloader::CachedScript>& scripts) > +{ > + if (!mWantCacheData) { Does `Send__delete__` destroy `this`? If so how do we get to this point? ::: js/xpconnect/loader/ScriptCacheActors.cpp:53 (Diff revision 2) > + auto data = dataArray.AppendElement(); > + > + data->url() = script->mURL; > + data->cachePath() = script->mCachePath; > + > + if (script->HasBuffer()) { Just to be clear, `HasBuffer` is only true if this is a script that wasn't in the cache right? If so can you add a note, if not I think we need to make sure we don't send up unnecessary data. ::: js/xpconnect/loader/ScriptCacheActors.cpp:60 (Diff revision 2) > + data->xdrData().AppendElements(xdrData.begin(), xdrData.length()); > + script->FreeData(); > + } > + } > + > + Send__delete__(this, dataArray); How much data are we sending? Is this a possible OOM point? It might make sense to send individually. ::: js/xpconnect/loader/ScriptCacheActors.cpp:67 (Diff revision 2) > + > +void > +ScriptCacheChild::ActorDestroy(ActorDestroyReason aWhy) > +{ > + auto& cache = ScriptPreloader::GetChildSingleton(); > + cache.mChildActor = nullptr; Maybe add a `clearChild` method rather than making this a friend? ::: js/xpconnect/loader/ScriptCacheActors.cpp:75 (Diff revision 2) > + > +IPCResult > +ScriptCacheParent::Recv__delete__(nsTArray<ScriptData>&& scripts) > +{ > + if (!mWantCacheData && scripts.Length()) { > + return IPC_FAIL(this, "UnexpectedScriptData"); I don't recall the details, but does `IPC_FAIL` cause a crash? If so we might just want to warn and ignore the data. ::: js/xpconnect/loader/ScriptPreloader.h:78 (Diff revision 2) > // Notes the execution of a script with the given URL and cache key. > // Depending on the stage of startup, the script may be serialized and > // stored to the startup script cache. > void NoteScript(const nsCString& url, const nsCString& cachePath, JS::HandleScript script); > > + void NoteScript(const nsCString& url, const nsCString& cachePath, Can you add a doc here indicating this is coming from the child process? ::: js/xpconnect/loader/ScriptPreloader.h:261 (Diff revision 2) > // existing cache file, or generated by encoding a script which was > // compiled during this session. > Maybe<JS::TranscodeRange> mXDRRange; > > // XDR data which was generated from a script compiled during this > // session, and will be written to the cache file. It might make sense to document what cases we have TranscodeBuffer vs nsTArray. ::: js/xpconnect/loader/ScriptPreloader.h:360 (Diff revision 2) > + // The process types for which remote processes have been initialized, and > + // are expected to send back script data. > + EnumSet<ProcessType> mInitializedProcesses{}; > + > RefPtr<ScriptPreloader> mChildCache; > + ScriptCacheChild* mChildActor = nullptr; I'm a little concerned about the lifetime of `mChildActor`. Are we guaranteed it's not going to die on us? ::: js/xpconnect/loader/ScriptPreloader.cpp:119 (Diff revision 2) > > return *singleton; > } > > +void > +ScriptPreloader::InitContentChild(ContentParent* parent) Can `parent` be const or is `SendPScriptCacheConstructor` non-const? Also would it make sense to take a ref or NotNull instead? ::: js/xpconnect/loader/ScriptPreloader.cpp:121 (Diff revision 2) > } > > +void > +ScriptPreloader::InitContentChild(ContentParent* parent) > +{ > + auto& cache = GetChildSingleton(); I got confused here, so this is code that's called in the parent process. It might be worth noting / asserting that. I don't feel too strongly about this though. Another thing that tripped me up is we use the term `GetChildSingleton` which is kind of right, but maybe `GetChildrenSingleton` makes more sense since we store data for all child process types in one singleton? Again I don't feel too strongly about this.
Attachment #8864351 -
Flags: review?(erahm) → review-
Assignee | ||
Comment 44•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review141746 > I'm assuming ipdl fields are optional? Or I guess we can do a zero-length array... Might be worth documenting that this isn't always used. They're not optional, and the only way to make them optional is to use a union. I'm sending a zero-length array for the case when we have no XDR data to send. > Not sure if I remember/understand this properly, but in the `wantCacheData == true` case is it possible the IPC message will timeout? No, IPDL messages in the content process should be basically infallible. If they fail, the process is killed. > Does `Send__delete__` destroy `this`? If so how do we get to this point? Yes. This should be an assertion, I guess. > Just to be clear, `HasBuffer` is only true if this is a script that wasn't in the cache right? > > If so can you add a note, if not I think we need to make sure we don't send up unnecessary data. Correct. I'll add a note. > How much data are we sending? Is this a possible OOM point? It might make sense to send individually. In the worst case, about 2MB for a fresh profile. There's probably not much point in sending them individually, since we need them all in memory in the parent for the write, anyway. > Maybe add a `clearChild` method rather than making this a friend? I'd rather it be a friend anyway. Nothing else should be accessing those members or methods. > I don't recall the details, but does `IPC_FAIL` cause a crash? If so we might just want to warn and ignore the data. It causes the child to be killed, but not the parent, which is what we want in this case. > I'm a little concerned about the lifetime of `mChildActor`. Are we guaranteed it's not going to die on us? The actor nulls out this property when it's destroyed, and we always check it before trying to use it, so this should be perfectly safe. > Can `parent` be const or is `SendPScriptCacheConstructor` non-const? Also would it make sense to take a ref or NotNull instead? `Send*` methods are always non-const. A ref would probably be a good idea. > I got confused here, so this is code that's called in the parent process. It might be worth noting / asserting that. > > I don't feel too strongly about this though. > > Another thing that tripped me up is we use the term `GetChildSingleton` which is kind of right, but maybe `GetChildrenSingleton` makes more sense since we store data for all child process types in one singleton? > > Again I don't feel too strongly about this. I'll add some notes. It should be ChildSingleton rather than ChildrenSingleton, since every child has its own child singleton, as does the parent. This is similar to how we have an in-process ContentChild in the parent process. But I'll add some notes about that too.
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8864354 [details] Bug 1361900: Part 9 - Sort scripts by initial load time before saving. https://reviewboard.mozilla.org/r/136012/#review141806 Looks good, just a few questions. r+ either way. ::: js/xpconnect/loader/ScriptPreloader.h:152 (Diff revision 2) > MOZ_ASSERT_IF(hashValue, hashValue == this); > #endif > mCache->mScripts.Remove(mCachePath); > } > > + struct Comparator Can you give this a more descriptive name? That `Equals` is not what I'd expect for comparing two scripts. Looking at `nsTArray::Sort` I get what's going on, but a note here would be good. I guess the idea is to sort things by: a) Primary: Async scripts first b) Secondary: Earlier load times first ::: js/xpconnect/loader/ScriptPreloader.h:181 (Diff revision 2) > mXDRRange.reset(); > mXDRData.destroy(); > } > } > > + void UpdateLoadTime(const TimeStamp& loadTime) Just double-checking: comparing TimeStamps across processes is legit, right? As in the timestamp isn't "time since the process started" or something like that. ::: js/xpconnect/loader/ScriptPreloader.cpp:602 (Diff revision 2) > } > > AutoFDClose fd; > NS_TRY(cacheFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE, 0644, &fd.rwget())); > > + nsTArray<CachedScript*> scripts; If we have a guesstimate on how many scripts there are it might make sense to reserve some space ahead of time.
Attachment #8864354 -
Flags: review?(erahm) → review+
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8864355 [details] Bug 1361900: Part 10 - Replace linked lists with a single hashtable. https://reviewboard.mozilla.org/r/136014/#review141816 One minor fix needed, just some comments on possible changes but I don't full super strongly about any of them. ::: js/xpconnect/loader/ScriptPreloader-inl.h:215 (Diff revision 2) > > + > +template <typename T> struct Matcher; > + > +template <typename T> > +class HashIter So if I'm reading this right this is a wrapper over the hash iterator that: a) doesn't care about the key b) can filter elements c) can be used in ranged based for loop First off: nice addition! Might make sense to do a follow up to add it to the xpcom class. Secondly: Perhaps a more specific name might be useful: `HashElemIter` or `ElemIter` or `FilterIter` or ... I could go on. Just somethign a little more specific. Can you also add a small comment describing the purpose of the class? ::: js/xpconnect/loader/ScriptPreloader-inl.h:225 (Diff revision 2) > + T& hash_; > + Matcher<ElemType>* matcher_; > + Maybe<Iterator> iter_; > + > +public: > + explicit HashIter(T& hash, Matcher<ElemType>* matcher = nullptr) Instead of a pointer it might be a little cleaner to just have a DefaultMatcher which always returns true. ie: ``` DefaultMatcher<T> : public Matcher<T> { Match(T) { return true; } }; explicit HashIter(T& hash, const Matcher<ElemType>& matcher = DefaultMatcher<T>()) ``` Or you could template on the matcher. I don't really have a strong opinion on this though. ::: js/xpconnect/loader/ScriptPreloader-inl.h:245 (Diff revision 2) > + {} > + > + Iterator& iter() { return iter_.iter_.ref(); } > + > + public: > + Elem& operator*() { return *this; } I don't totally follow this, why define an `operator *` that just does the default `operator *` behavior? Or did you mean to do `ElemType&`? ::: js/xpconnect/loader/ScriptPreloader-inl.h:279 (Diff revision 2) > + > + Elem end() { return Elem(*this, true); } > +}; > + > +template <typename T> > +HashIter<T> IterHash(T& hash, Matcher<typename T::UserDataType>* matcher = nullptr) This name is a bit confusing given1 the `HashIter` type. I don't have much more to say than that. Why not just call `HashIter` directly? ::: js/xpconnect/loader/ScriptPreloader.h:174 (Diff revision 2) > } > return a->mLoadTime < b->mLoadTime; > } > }; > > + struct ScriptMatcher final : public Matcher<CachedScript*> Maybe `ScriptStatusMatcher` or just `StatusMatcher` instead? ::: js/xpconnect/loader/ScriptPreloader.cpp:65 (Diff revision 2) > "Memory used to hold the scripts which have been executed in this " > "session, and will be written to the startup script cache file."); > > MOZ_COLLECT_REPORT( > "explicit/script-preloader/heap/restored-scripts", KIND_HEAP, UNITS_BYTES, > - SizeOfLinkedList(mRestoredScripts, MallocSizeOf), > + SizeOfHashEntries<ScriptStatus::Saved>(mScripts, MallocSizeOf), Should be `ScriptStatus::Restored`. ::: js/xpconnect/loader/ScriptPreloader.cpp:230 (Diff revision 2) > while (!mSaveComplete && mSaveThread) { > mal.Wait(); > } > } > > - mSavedScripts.clear(); > + mScripts.Clear(); Just checking: this isn't leaking because we use a nsClassHashtable now and that owns the elements?
Attachment #8864355 -
Flags: review?(erahm) → review-
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864355 [details] Bug 1361900: Part 10 - Replace linked lists with a single hashtable. https://reviewboard.mozilla.org/r/136014/#review141816 > So if I'm reading this right this is a wrapper over the hash iterator that: > a) doesn't care about the key > b) can filter elements > c) can be used in ranged based for loop > > First off: nice addition! Might make sense to do a follow up to add it to the xpcom class. > > Secondly: Perhaps a more specific name might be useful: `HashElemIter` or `ElemIter` or `FilterIter` or ... I could go on. Just somethign a little more specific. Can you also add a small comment describing the purpose of the class? I'll file a separate bug to move it to a shared helper. > Instead of a pointer it might be a little cleaner to just have a DefaultMatcher which always returns true. ie: > > ``` > DefaultMatcher<T> : public Matcher<T> { Match(T) { return true; } }; > > explicit HashIter(T& hash, const Matcher<ElemType>& matcher = DefaultMatcher<T>()) > ``` > > Or you could template on the matcher. I don't really have a strong opinion on this though. I thought about it, but it wound up being more complicated than the simple null check, and the null check at least lets us avoid the extra virtual call when we don't have a filter. > I don't totally follow this, why define an `operator *` that just does the default `operator *` behavior? Or did you mean to do `ElemType&`? Because the range iterator protocol requires that iterator element be dereferenced at each iteration so that it works transparently for things like pointer ranges. This is a pretty common, if unfortunate, pattern in range iterator classes. > This name is a bit confusing given1 the `HashIter` type. I don't have much more to say than that. Why not just call `HashIter` directly? Because instantiating a template class requires all non-default template arguments to be explicitly specified, which gets extremely noisy, especially when we're also specifying a filter. > Maybe `ScriptStatusMatcher` or just `StatusMatcher` instead? Yeah, that probably makes sense. > Should be `ScriptStatus::Restored`. Ugh... Thanks for catching that. > Just checking: this isn't leaking because we use a nsClassHashtable now and that owns the elements? Yes. The elements are destroyed when they're removed from the hash unless they're removed with RemoveAndForget.
Assignee | ||
Comment 48•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864354 [details] Bug 1361900: Part 9 - Sort scripts by initial load time before saving. https://reviewboard.mozilla.org/r/136012/#review141806 > Can you give this a more descriptive name? That `Equals` is not what I'd expect for comparing two scripts. Looking at `nsTArray::Sort` I get what's going on, but a note here would be good. > > I guess the idea is to sort things by: > a) Primary: Async scripts first > b) Secondary: Earlier load times first I can't rename, because those are the methods the Sort methods requires. I can add comments, though. > Just double-checking: comparing TimeStamps across processes is legit, right? As in the timestamp isn't "time since the process started" or something like that. On Unix-ish systems, it definitely is. On Windows, I *think* it is, but I'm not 100% sure. I'll confirm before landing.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 51•7 years ago
|
||
mozreview-review |
Comment on attachment 8864355 [details] Bug 1361900: Part 10 - Replace linked lists with a single hashtable. https://reviewboard.mozilla.org/r/136014/#review142178 \o/
Attachment #8864355 -
Flags: review?(erahm) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 59•7 years ago
|
||
mozreview-review |
Comment on attachment 8864351 [details] Bug 1361900: Part 6 - Add content process support for the script preloader. https://reviewboard.mozilla.org/r/136006/#review142194
Attachment #8864351 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 60•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8864346 [details] Bug 1361900: Part 1 - Make CDMProxy.h compatible with mozilla::Result. https://reviewboard.mozilla.org/r/135996/#review139064 > Avoid using namespace in header file. Maybe you can use it once Cpp wants to use it. This isn't `using namespace`, just `using <type>` so the type is still availabile as `mozilla::DecryptStatus` but its enum values aren't. > Why using namespace here? I think I added it before I added the `using DecryptStatus` to the header file, so the type would be available. > I'm not sure why you only specify the eme only on Ok(since it is too common that makes you compile failure?). I suggest not using namespace eme but writing eme::Ok, eme::NoKeyErr, eme::AbortedErr instead for consistency. I was just trying to minimize the changes, but I'm happy to specify it for the rest of the constants too. > IIUC, don't need to using since you specify the eme:: below? It was still necessary for NoKeyErr, but I changed that to explicitly use eme::
Assignee | ||
Comment 61•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3064aec4bb16542f9cc93a89f7a257b6716a98 Bug 1361900: Part 1 - Make CDMProxy.h compatible with mozilla::Result. r=JamesCheng https://hg.mozilla.org/integration/mozilla-inbound/rev/39ffa8a7801a7807a1097ea2c9290c772cd54a9b Bug 1361900: Part 2 - Add process types field to cached script data. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/bd55bd27bb1e55e44fbe76786993b1657c2af944 Bug 1361900: Part 3 - Add a helper script to decode and inspect script cache data. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/ad243db647c708b74859d73e8787b5b34897c140 Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. r=erahm,gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/540ecb4c1f0fc25725375c7cc5a399a1d2bea5fe Bug 1361900: Part 5 - Add support for IPC FileDescriptors to AutoMemMap. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/4227bcda00babd2d5980dca3b42cb4b467da38c4 Bug 1361900: Part 6 - Add content process support for the script preloader. r=erahm,gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/3c716b3b45417e45100d5480241921f44c601270 Bug 1361900: Part 7 - Use the script preloader in content processes in subscript and component loaders. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/752295ce12d674f4b5e879c258d4c82fec572484 Bug 1361900: Part 8 - Use the script preloader in content processes in the frame script loader. r=gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/2a48f8b743d158bc744a776bd939c80a242e1446 Bug 1361900: Part 9 - Sort scripts by initial load time before saving. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/75b28187fa5ed70fafdd30565a12584b301b9104 Bug 1361900: Part 10 - Replace linked lists with a single hashtable. r=erahm
Assignee | ||
Comment 62•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9795dacd97e1dafdfc23ad1f2188cca3c2b9f45e Bug 1361900: Follow-up: Fix rebase botch. r=me
Assignee | ||
Comment 63•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d47499f43b3f509a28f2ed3a68f82e919a318cb Bug 1361900: Follow-up: Fix another rebase botch. r=me
Assignee | ||
Comment 64•7 years ago
|
||
Sorry for the rebase botches. I wound up rebasing the fixes for the static analysis issues onto the patches for bug 1363482.
Assignee | ||
Comment 65•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b38ac21c7a5ce6141909e13c6cd9d0a18965438 Bug 1361900: Follow-up: Fix order of execution issue. r=me
Assignee | ||
Comment 66•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/23dc7b443bb0528ec880acb7fc0a32aec05f0255 Bug 1361900: Fix one last merge botch, and fix a false rooting hazard warning.
Comment 67•7 years ago
|
||
Backed out for crashing Marionette's test_timeouts.py TestTimeouts.test_reset_timeout on Linux debug with e10s: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c35b4cb9a31b34b00be41b023bc72c6d8f8cff3 https://hg.mozilla.org/integration/mozilla-inbound/rev/17bc47bdb8ca0e05dfdb13413eeecd9358b4feb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ecc99173e52bdec143f8a8c764eb7d2498148b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7060a061f0cee36dfc54e053323166d0cd8944 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fc6eeeb12e9c1de8271a9cb95935411acf011c https://hg.mozilla.org/integration/mozilla-inbound/rev/563613c57b6065366c74e8f8e25026aa4e7cebad https://hg.mozilla.org/integration/mozilla-inbound/rev/0c55713c1af4d1f1bbbfbc6f4c909726c6ad78c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/65df716e6fca85c9d08520b68f9a4aac6125fcef https://hg.mozilla.org/integration/mozilla-inbound/rev/f6eab92eb426784139ff901ff085025c8e6a8974 https://hg.mozilla.org/integration/mozilla-inbound/rev/96ef5ea12d40c5a7c3972fb1d08d2cca0338dd52 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb23534c2aa048fd9802b1d61bbc895ddf431fc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f81447b0e728245d312d0749cd3991576fc4447 https://hg.mozilla.org/integration/mozilla-inbound/rev/e19f52ae115ada27a8e1ab301498fce5a30c99c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb1a39c9977c4cb057e96244498ba1a1272a0fe Inbound's tip with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=17411eb12bd5fadbf4398592bfb8e9a8ff9abecb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=98896432&repo=mozilla-inbound [task 2017-05-13T15:39:57.170109Z] 15:39:57 INFO - TEST-START | test_timeouts.py TestTimeouts.test_reset_timeout [task 2017-05-13T15:39:58.024840Z] 15:39:58 INFO - Application command: /home/worker/workspace/build/application/firefox/firefox -no-remote -marionette -profile /tmp/tmpSFp5OV.mozrunner [task 2017-05-13T15:41:58.038936Z] 15:41:58 INFO - mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmpSFp5OV.mozrunner/minidumps/6abb3bbc-cee3-2acd-fd88-6b681791d864.dmp /home/worker/workspace/build/symbols [task 2017-05-13T15:42:05.475301Z] 15:42:05 INFO - mozcrash Saved minidump as /home/worker/workspace/build/blobber_upload_dir/6abb3bbc-cee3-2acd-fd88-6b681791d864.dmp [task 2017-05-13T15:42:05.476829Z] 15:42:05 INFO - mozcrash Saved app info as /home/worker/workspace/build/blobber_upload_dir/6abb3bbc-cee3-2acd-fd88-6b681791d864.extra [task 2017-05-13T15:42:05.870836Z] 15:42:05 INFO - PROCESS-CRASH | test_timeouts.py TestTimeouts.test_reset_timeout | application crashed [@ mozilla::TimeStamp::operator<] [task 2017-05-13T15:42:05.871860Z] 15:42:05 INFO - Crash dump filename: /tmp/tmpSFp5OV.mozrunner/minidumps/6abb3bbc-cee3-2acd-fd88-6b681791d864.dmp [task 2017-05-13T15:42:05.872685Z] 15:42:05 INFO - Operating system: Linux [task 2017-05-13T15:42:05.873599Z] 15:42:05 INFO - 0.0.0 Linux 3.13.0-112-generic #159-Ubuntu SMP Fri Mar 3 15:26:07 UTC 2017 x86_64 [task 2017-05-13T15:42:05.874715Z] 15:42:05 INFO - CPU: amd64 [task 2017-05-13T15:42:05.875508Z] 15:42:05 INFO - family 6 model 62 stepping 4 [task 2017-05-13T15:42:05.876326Z] 15:42:05 INFO - 2 CPUs [task 2017-05-13T15:42:05.877143Z] 15:42:05 INFO - [task 2017-05-13T15:42:05.877892Z] 15:42:05 INFO - GPU: UNKNOWN [task 2017-05-13T15:42:05.878668Z] 15:42:05 INFO - [task 2017-05-13T15:42:05.879486Z] 15:42:05 INFO - Crash reason: SIGSEGV [task 2017-05-13T15:42:05.880323Z] 15:42:05 INFO - Crash address: 0x0 [task 2017-05-13T15:42:05.881138Z] 15:42:05 INFO - Process uptime: not available [task 2017-05-13T15:42:05.881900Z] 15:42:05 INFO - [task 2017-05-13T15:42:05.882717Z] 15:42:05 INFO - Thread 0 (crashed) [task 2017-05-13T15:42:05.883514Z] 15:42:05 INFO - 0 libxul.so!mozilla::TimeStamp::operator< [TimeStamp.h:17411eb12bd5 : 555 + 0x1d] [task 2017-05-13T15:42:05.884231Z] 15:42:05 INFO - rax = 0x0000000000000000 rdx = 0x0000000000000000 [task 2017-05-13T15:42:05.884960Z] 15:42:05 INFO - rcx = 0x00007fbdfd7026fd rbx = 0x00007fbde5b12900 [task 2017-05-13T15:42:05.885720Z] 15:42:05 INFO - rsi = 0x00007fbdfd9d1770 rdi = 0x00007fbdfd9d0540 [task 2017-05-13T15:42:05.886445Z] 15:42:05 INFO - rbp = 0x00007ffd9e7ff810 rsp = 0x00007ffd9e7ff810 [task 2017-05-13T15:42:05.887173Z] 15:42:05 INFO - r8 = 0x00007fbdfd9d1770 r9 = 0x00007fbdfea99740 [task 2017-05-13T15:42:05.887882Z] 15:42:05 INFO - r10 = 0x000000000000004a r11 = 0x0000000000000000 [task 2017-05-13T15:42:05.888640Z] 15:42:05 INFO - r12 = 0x00007ffd9e7ff858 r13 = 0x00007fbde581e000 [task 2017-05-13T15:42:05.889350Z] 15:42:05 INFO - r14 = 0x00007fbde5977000 r15 = 0x00007fbde581e000 [task 2017-05-13T15:42:05.890071Z] 15:42:05 INFO - rip = 0x00007fbded924032 [task 2017-05-13T15:42:05.890776Z] 15:42:05 INFO - Found by: given as instruction pointer in context [task 2017-05-13T15:42:05.891504Z] 15:42:05 INFO - 1 libxul.so!mozilla::ScriptPreloader::CachedScript::UpdateLoadTime [ScriptPreloader.h:17411eb12bd5 : 211 + 0xc] [task 2017-05-13T15:42:05.892227Z] 15:42:05 INFO - rbx = 0x00007fbde5b12900 rbp = 0x00007ffd9e7ff830 [task 2017-05-13T15:42:05.892931Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff820 r12 = 0x00007ffd9e7ff858 [task 2017-05-13T15:42:05.893682Z] 15:42:05 INFO - r13 = 0x00007fbde581e000 r14 = 0x00007fbde5977000 [task 2017-05-13T15:42:05.894418Z] 15:42:05 INFO - r15 = 0x00007fbde581e000 rip = 0x00007fbdedfe3bba [task 2017-05-13T15:42:05.895168Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.895875Z] 15:42:05 INFO - 2 libxul.so!mozilla::ScriptPreloader::NoteScript [ScriptPreloader.cpp:17411eb12bd5 : 725 + 0xc] [task 2017-05-13T15:42:05.896567Z] 15:42:05 INFO - rbx = 0x00007fbdcc91b830 rbp = 0x00007ffd9e7ff8a0 [task 2017-05-13T15:42:05.897312Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff840 r12 = 0x00007fbde5b12900 [task 2017-05-13T15:42:05.898026Z] 15:42:05 INFO - r13 = 0x00007fbde581e000 r14 = 0x00007fbde5977000 [task 2017-05-13T15:42:05.898722Z] 15:42:05 INFO - r15 = 0x00007fbde581e000 rip = 0x00007fbdedfeaf9f [task 2017-05-13T15:42:05.899398Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.900132Z] 15:42:05 INFO - 3 libxul.so!mozilla::loader::ScriptCacheParent::Recv__delete__ [ScriptCacheActors.cpp:17411eb12bd5 : 89 + 0x9] [task 2017-05-13T15:42:05.900833Z] 15:42:05 INFO - rbx = 0x0000000000000001 rbp = 0x00007ffd9e7ff8e0 [task 2017-05-13T15:42:05.901552Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff8b0 r12 = 0x00007ffd9e7ff908 [task 2017-05-13T15:42:05.902270Z] 15:42:05 INFO - r13 = 0x0000000000000020 r14 = 0x00007ffd9e7ff901 [task 2017-05-13T15:42:05.902984Z] 15:42:05 INFO - r15 = 0x00007fbde581e000 rip = 0x00007fbdedfeb058 [task 2017-05-13T15:42:05.903679Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.904398Z] 15:42:05 INFO - 4 libxul.so!mozilla::loader::PScriptCacheParent::OnMessageReceived [PScriptCacheParent.cpp:17411eb12bd5 : 84 + 0xf] [task 2017-05-13T15:42:05.905129Z] 15:42:05 INFO - rbx = 0x00007fbdd5cc38b0 rbp = 0x00007ffd9e7ff970 [task 2017-05-13T15:42:05.905840Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff8f0 r12 = 0x00007ffd9e7ff920 [task 2017-05-13T15:42:05.906522Z] 15:42:05 INFO - r13 = 0x00007fbdccadf358 r14 = 0x00007ffd9e7ff908 [task 2017-05-13T15:42:05.907179Z] 15:42:05 INFO - r15 = 0x00007fbdccadf360 rip = 0x00007fbdede139cc [task 2017-05-13T15:42:05.907860Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.908537Z] 15:42:05 INFO - 5 libxul.so!mozilla::ipc::MessageChannel::DispatchAsyncMessage [MessageChannel.cpp:17411eb12bd5 : 2040 + 0x6] [task 2017-05-13T15:42:05.909220Z] 15:42:05 INFO - rbx = 0x00007fbdd5cc4120 rbp = 0x00007ffd9e7ff9b0 [task 2017-05-13T15:42:05.909940Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff980 r12 = 0x00007fbdccadf358 [task 2017-05-13T15:42:05.910632Z] 15:42:05 INFO - r13 = 0x0000000000000001 r14 = 0x00007ffd9e7ffa00 [task 2017-05-13T15:42:05.911342Z] 15:42:05 INFO - r15 = 0x0000000000000000 rip = 0x00007fbdedcd915c [task 2017-05-13T15:42:05.912048Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.912763Z] 15:42:05 INFO - 6 libxul.so!mozilla::ipc::MessageChannel::DispatchMessage [MessageChannel.cpp:17411eb12bd5 : 1975 + 0xb] [task 2017-05-13T15:42:05.913467Z] 15:42:05 INFO - rbx = 0x00007fbdccadf358 rbp = 0x00007ffd9e7ffa60 [task 2017-05-13T15:42:05.914292Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ff9c0 r12 = 0x00007fbdd5cc4120 [task 2017-05-13T15:42:05.915050Z] 15:42:05 INFO - r13 = 0x00007ffd9e7ff9d0 r14 = 0x00007ffd9e7ffa08 [task 2017-05-13T15:42:05.915758Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ff9e0 rip = 0x00007fbdedce3854 [task 2017-05-13T15:42:05.916457Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.917245Z] 15:42:05 INFO - 7 libxul.so!mozilla::ipc::MessageChannel::RunMessage [MessageChannel.cpp:17411eb12bd5 : 1844 + 0xb] [task 2017-05-13T15:42:05.917992Z] 15:42:05 INFO - rbx = 0x00007fbdd5cc4120 rbp = 0x00007ffd9e7ffac0 [task 2017-05-13T15:42:05.918696Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffa70 r12 = 0x00007fbdccadf358 [task 2017-05-13T15:42:05.919397Z] 15:42:05 INFO - r13 = 0x00007fbdccadf330 r14 = 0x00007fbdccadf300 [task 2017-05-13T15:42:05.920103Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffbef rip = 0x00007fbdedce5ba1 [task 2017-05-13T15:42:05.920807Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.921571Z] 15:42:05 INFO - 8 libxul.so!mozilla::ipc::MessageChannel::MessageTask::Run [MessageChannel.cpp:17411eb12bd5 : 1877 + 0xc] [task 2017-05-13T15:42:05.922297Z] 15:42:05 INFO - rbx = 0x00007fbdccadf300 rbp = 0x00007ffd9e7ffaf0 [task 2017-05-13T15:42:05.923003Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffad0 r12 = 0x00007fbdd5cc4140 [task 2017-05-13T15:42:05.923752Z] 15:42:05 INFO - r13 = 0x00007ffd9e7ffb78 r14 = 0x00007ffd9e7ffb20 [task 2017-05-13T15:42:05.924443Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffbef rip = 0x00007fbdedce5d13 [task 2017-05-13T15:42:05.925135Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.925894Z] 15:42:05 INFO - 9 libxul.so!nsThread::ProcessNextEvent [nsThread.cpp:17411eb12bd5 : 1270 + 0x6] [task 2017-05-13T15:42:05.926610Z] 15:42:05 INFO - rbx = 0x00007fbdeb17a160 rbp = 0x00007ffd9e7ffbd0 [task 2017-05-13T15:42:05.927302Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffb00 r12 = 0x00007ffd9e7ffb40 [task 2017-05-13T15:42:05.927996Z] 15:42:05 INFO - r13 = 0x00007ffd9e7ffb78 r14 = 0x00007ffd9e7ffb20 [task 2017-05-13T15:42:05.928703Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffbef rip = 0x00007fbded937f66 [task 2017-05-13T15:42:05.929445Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.930175Z] 15:42:05 INFO - 10 libxul.so!NS_ProcessNextEvent [nsThreadUtils.cpp:17411eb12bd5 : 393 + 0x11] [task 2017-05-13T15:42:05.930880Z] 15:42:05 INFO - rbx = 0x00007fbdeb17a160 rbp = 0x00007ffd9e7ffc00 [task 2017-05-13T15:42:05.931607Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffbe0 r12 = 0x0000000000000000 [task 2017-05-13T15:42:05.932322Z] 15:42:05 INFO - r13 = 0x00007fbdfd4d1df0 r14 = 0x00007fbdeb17a160 [task 2017-05-13T15:42:05.933061Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffd01 rip = 0x00007fbded93a148 [task 2017-05-13T15:42:05.933845Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.934601Z] 15:42:05 INFO - 11 libxul.so!mozilla::ipc::MessagePump::Run [MessagePump.cpp:17411eb12bd5 : 96 + 0xa] [task 2017-05-13T15:42:05.935351Z] 15:42:05 INFO - rbx = 0x00007fbdfd4d1dc0 rbp = 0x00007ffd9e7ffc70 [task 2017-05-13T15:42:05.936077Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffc10 r12 = 0x00007fbdeb1129e0 [task 2017-05-13T15:42:05.936763Z] 15:42:05 INFO - r13 = 0x00007fbdfd4d1df0 r14 = 0x00007fbdeb17a160 [task 2017-05-13T15:42:05.937498Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffd01 rip = 0x00007fbdedcdef61 [task 2017-05-13T15:42:05.938206Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.938912Z] 15:42:05 INFO - 12 libxul.so!MessageLoop::RunInternal [message_loop.cc:17411eb12bd5 : 238 + 0x17] [task 2017-05-13T15:42:05.939604Z] 15:42:05 INFO - rbx = 0x00007fbdeb1129e0 rbp = 0x00007ffd9e7ffcb0 [task 2017-05-13T15:42:05.940302Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffc80 r12 = 0x00007fbdeb17a160 [task 2017-05-13T15:42:05.941016Z] 15:42:05 INFO - r13 = 0x0000000000000077 r14 = 0x00007ffd9e7ffd98 [task 2017-05-13T15:42:05.941773Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffdc8 rip = 0x00007fbdedcae0ff [task 2017-05-13T15:42:05.942457Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.943178Z] 15:42:05 INFO - 13 libxul.so!MessageLoop::Run [message_loop.cc:17411eb12bd5 : 231 + 0x8] [task 2017-05-13T15:42:05.943897Z] 15:42:05 INFO - rbx = 0x00007fbdeb1129e0 rbp = 0x00007ffd9e7ffcf0 [task 2017-05-13T15:42:05.944605Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffcc0 r12 = 0x00007fbdeb17a160 [task 2017-05-13T15:42:05.945314Z] 15:42:05 INFO - r13 = 0x0000000000000077 r14 = 0x00007ffd9e7ffd98 [task 2017-05-13T15:42:05.946059Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffdc8 rip = 0x00007fbdedcae126 [task 2017-05-13T15:42:05.946749Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.947446Z] 15:42:05 INFO - 14 libxul.so!nsBaseAppShell::Run [nsBaseAppShell.cpp:17411eb12bd5 : 156 + 0xd] [task 2017-05-13T15:42:05.948131Z] 15:42:05 INFO - rbx = 0x00007fbde46dfbe0 rbp = 0x00007ffd9e7ffd10 [task 2017-05-13T15:42:05.948830Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffd00 r12 = 0x00007fbdeb17a160 [task 2017-05-13T15:42:05.949535Z] 15:42:05 INFO - r13 = 0x0000000000000077 r14 = 0x00007ffd9e7ffd98 [task 2017-05-13T15:42:05.950257Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffdc8 rip = 0x00007fbdef3877d9 [task 2017-05-13T15:42:05.950927Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.951628Z] 15:42:05 INFO - 15 libxul.so!nsAppStartup::Run [nsAppStartup.cpp:17411eb12bd5 : 283 + 0x6] [task 2017-05-13T15:42:05.952332Z] 15:42:05 INFO - rbx = 0x00007fbde44dc240 rbp = 0x00007ffd9e7ffd30 [task 2017-05-13T15:42:05.953408Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffd20 r12 = 0x00007ffd9e7ffde0 [task 2017-05-13T15:42:05.954194Z] 15:42:05 INFO - r13 = 0x0000000000000077 r14 = 0x00007ffd9e7ffd98 [task 2017-05-13T15:42:05.954897Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffdc8 rip = 0x00007fbdf016cfa9 [task 2017-05-13T15:42:05.955635Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.956376Z] 15:42:05 INFO - 16 libxul.so!XREMain::XRE_mainRun [nsAppRunner.cpp:17411eb12bd5 : 4553 + 0x11] [task 2017-05-13T15:42:05.957037Z] 15:42:05 INFO - rbx = 0x00007ffd9e7ffe20 rbp = 0x00007ffd9e7ffeb0 [task 2017-05-13T15:42:05.957757Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffd40 r12 = 0x00007ffd9e7ffde0 [task 2017-05-13T15:42:05.958470Z] 15:42:05 INFO - r13 = 0x0000000000000077 r14 = 0x00007ffd9e7ffd98 [task 2017-05-13T15:42:05.959180Z] 15:42:05 INFO - r15 = 0x00007ffd9e7ffdc8 rip = 0x00007fbdf01ecf44 [task 2017-05-13T15:42:05.959865Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.960579Z] 15:42:05 INFO - 17 libxul.so!XREMain::XRE_main [nsAppRunner.cpp:17411eb12bd5 : 4733 + 0x5] [task 2017-05-13T15:42:05.961273Z] 15:42:05 INFO - rbx = 0x00007ffd9e7fff80 rbp = 0x00007ffd9e7fff60 [task 2017-05-13T15:42:05.962047Z] 15:42:05 INFO - rsp = 0x00007ffd9e7ffec0 r12 = 0x0000000000000000 [task 2017-05-13T15:42:05.962755Z] 15:42:05 INFO - r13 = 0x00007ffd9e7ffff8 r14 = 0x0000000000000001 [task 2017-05-13T15:42:05.963464Z] 15:42:05 INFO - r15 = 0x00007ffd9e7fff08 rip = 0x00007fbdf01ed6e3 [task 2017-05-13T15:42:05.964165Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.964880Z] 15:42:05 INFO - 18 libxul.so!XRE_main [nsAppRunner.cpp:17411eb12bd5 : 4826 + 0x5] [task 2017-05-13T15:42:05.965579Z] 15:42:05 INFO - rbx = 0x00007ffd9e7fff80 rbp = 0x00007ffd9e800140 [task 2017-05-13T15:42:05.966333Z] 15:42:05 INFO - rsp = 0x00007ffd9e7fff70 r12 = 0x0000000000000005 [task 2017-05-13T15:42:05.967034Z] 15:42:05 INFO - r13 = 0x00007ffd9e801288 r14 = 0x00007ffd9e800150 [task 2017-05-13T15:42:05.967749Z] 15:42:05 INFO - r15 = 0x0000000000000000 rip = 0x00007fbdf01ed975 [task 2017-05-13T15:42:05.968421Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.969094Z] 15:42:05 INFO - 19 firefox!do_main [nsBrowserApp.cpp:17411eb12bd5 : 236 + 0x22] [task 2017-05-13T15:42:05.969810Z] 15:42:05 INFO - rbx = 0x0000000000000005 rbp = 0x00007ffd9e801170 [task 2017-05-13T15:42:05.970473Z] 15:42:05 INFO - rsp = 0x00007ffd9e800150 r12 = 0x00007ffd9e801288 [task 2017-05-13T15:42:05.971131Z] 15:42:05 INFO - r13 = 0x0000000000000000 r14 = 0x00007ffd9e8012b8 [task 2017-05-13T15:42:05.971803Z] 15:42:05 INFO - r15 = 0x0000000000000000 rip = 0x00000000004066d6 [task 2017-05-13T15:42:05.972484Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.973162Z] 15:42:05 INFO - 20 firefox!main [nsBrowserApp.cpp:17411eb12bd5 : 309 + 0xe] [task 2017-05-13T15:42:05.973885Z] 15:42:05 INFO - rbx = 0x00007ffd9e801288 rbp = 0x00007ffd9e8011a0 [task 2017-05-13T15:42:05.974635Z] 15:42:05 INFO - rsp = 0x00007ffd9e801180 r12 = 0x0000000000000005 [task 2017-05-13T15:42:05.981129Z] 15:42:05 INFO - r13 = 0x00007ffd9e8012b8 r14 = 0x000002e85974c625 [task 2017-05-13T15:42:05.981903Z] 15:42:05 INFO - r15 = 0x0000000000000000 rip = 0x0000000000405e80 [task 2017-05-13T15:42:05.982939Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.983795Z] 15:42:05 INFO - 21 libc-2.23.so + 0x20830 [task 2017-05-13T15:42:05.985873Z] 15:42:05 INFO - rbx = 0x0000000000000000 rbp = 0x000000000041df70 [task 2017-05-13T15:42:05.986882Z] 15:42:05 INFO - rsp = 0x00007ffd9e8011b0 r12 = 0x00000000004060c8 [task 2017-05-13T15:42:05.987711Z] 15:42:05 INFO - r13 = 0x00007ffd9e801280 r14 = 0x0000000000000000 [task 2017-05-13T15:42:05.988701Z] 15:42:05 INFO - r15 = 0x0000000000000000 rip = 0x00007fbdfd62c830 [task 2017-05-13T15:42:05.989516Z] 15:42:05 INFO - Found by: call frame info [task 2017-05-13T15:42:05.990548Z] 15:42:05 INFO - 22 firefox!MOZ_ReportAssertionFailure [Assertions.h:17411eb12bd5 : 164 + 0x5] [task 2017-05-13T15:42:05.991566Z] 15:42:05 INFO - rsp = 0x00007ffd9e8011d0 rip = 0x0000000000405df5 [task 2017-05-13T15:42:05.992365Z] 15:42:05 INFO - Found by: stack scanning [task 2017-05-13T15:42:05.993328Z] 15:42:05 INFO - [...] [task 2017-05-13T15:42:08.351283Z] 15:42:08 INFO - TEST-UNEXPECTED-ERROR | test_timeouts.py TestTimeouts.test_reset_timeout | IOError: Process crashed (Exit code: 11) (Reason: Timed out waiting for connection on localhost:2828!) [task 2017-05-13T15:42:08.352109Z] 15:42:08 INFO - Traceback (most recent call last): [task 2017-05-13T15:42:08.352998Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 166, in run [task 2017-05-13T15:42:08.353822Z] 15:42:08 INFO - testMethod() [task 2017-05-13T15:42:08.354660Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/decorators.py", line 82, in skip_wrapper [task 2017-05-13T15:42:08.355496Z] 15:42:08 INFO - return test_item(self, *args, **kwargs) [task 2017-05-13T15:42:08.356584Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/decorators.py", line 150, in skip_wrapper [task 2017-05-13T15:42:08.357385Z] 15:42:08 INFO - return test_item(self, *args, **kwargs) [task 2017-05-13T15:42:08.358228Z] 15:42:08 INFO - File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_timeouts.py", line 81, in test_reset_timeout [task 2017-05-13T15:42:08.359015Z] 15:42:08 INFO - do_check(self.marionette.restart) [task 2017-05-13T15:42:08.359875Z] 15:42:08 INFO - File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_timeouts.py", line 73, in do_check [task 2017-05-13T15:42:08.360664Z] 15:42:08 INFO - callback() [task 2017-05-13T15:42:08.361467Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 23, in _ [task 2017-05-13T15:42:08.362283Z] 15:42:08 INFO - return func(*args, **kwargs) [task 2017-05-13T15:42:08.363357Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 1268, in restart [task 2017-05-13T15:42:08.364175Z] 15:42:08 INFO - self.raise_for_port() [task 2017-05-13T15:42:08.365095Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 28, in _ [task 2017-05-13T15:42:08.365936Z] 15:42:08 INFO - m._handle_socket_failure() [task 2017-05-13T15:42:08.366753Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/decorators.py", line 23, in _ [task 2017-05-13T15:42:08.367948Z] 15:42:08 INFO - return func(*args, **kwargs) [task 2017-05-13T15:42:08.368750Z] 15:42:08 INFO - File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/marionette.py", line 697, in raise_for_port [task 2017-05-13T15:42:08.369573Z] 15:42:08 INFO - self.host, self.port)) [task 2017-05-13T15:42:08.370417Z] 15:42:08 INFO - TEST-INFO took 128536ms
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 68•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ba376994c494606fc3264f013615974d172bcd4 Bug 1361900: Part 1 - Make CDMProxy.h compatible with mozilla::Result. r=JamesCheng https://hg.mozilla.org/integration/mozilla-inbound/rev/8da0214dc2e555cbd2be2094454f616fbad1b1c5 Bug 1361900: Part 2 - Add process types field to cached script data. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/d7062f4cdbccc85df24082fe124cd1056fd5e644 Bug 1361900: Part 3 - Add a helper script to decode and inspect script cache data. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/43742d606a92a09ed1337d5c4cd98b8b7fcf6ff0 Bug 1361900: Part 4 - Use a separate script cache for scripts loaded in the child process. r=erahm,gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/31eb52f1973100820ff8c4165016a3c894af463d Bug 1361900: Part 5 - Add support for IPC FileDescriptors to AutoMemMap. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/a37bbe24afaa765156a87adc59b72b3ede8b6fe2 Bug 1361900: Part 6 - Add content process support for the script preloader. r=erahm,gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/21718f924d761ec8b624cdb48c0947535e37a072 Bug 1361900: Part 7 - Use the script preloader in content processes in subscript and component loaders. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcba8173e8293f35a88fd560da2f265848d99e4 Bug 1361900: Part 8 - Use the script preloader in content processes in the frame script loader. r=gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/7c86ed8263e12bdeddbd809e512c067bc343fa2d Bug 1361900: Part 9 - Sort scripts by initial load time before saving. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/906aee9cc744ab3fed98ede38756596263bc9319 Bug 1361900: Part 10 - Replace linked lists with a single hashtable. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa1c7e8bbf1bfbae3aac753e7a415be9e1859a3 Bug 1361900: Part 11 - Make sure new cache file is closed before renaming on Windows. r=me
Comment 69•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ba376994c49 https://hg.mozilla.org/mozilla-central/rev/8da0214dc2e5 https://hg.mozilla.org/mozilla-central/rev/d7062f4cdbcc https://hg.mozilla.org/mozilla-central/rev/43742d606a92 https://hg.mozilla.org/mozilla-central/rev/31eb52f19731 https://hg.mozilla.org/mozilla-central/rev/a37bbe24afaa https://hg.mozilla.org/mozilla-central/rev/21718f924d76 https://hg.mozilla.org/mozilla-central/rev/0fcba8173e82 https://hg.mozilla.org/mozilla-central/rev/7c86ed8263e1 https://hg.mozilla.org/mozilla-central/rev/906aee9cc744 https://hg.mozilla.org/mozilla-central/rev/2aa1c7e8bbf1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 70•7 years ago
|
||
mozreview-review |
Comment on attachment 8864346 [details] Bug 1361900: Part 1 - Make CDMProxy.h compatible with mozilla::Result. https://reviewboard.mozilla.org/r/135996/#review142338
Comment 71•7 years ago
|
||
it is unfortunate that the valid comment/concern was mentioned for the first patch, yet was dismissed.
Assignee | ||
Comment 72•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #71) > it is unfortunate that the valid comment/concern was mentioned for the first > patch, yet was dismissed. Which concern was that? I'm fairly certain I addressed every comment in that review.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jyavenard)
Comment 73•7 years ago
|
||
It appears I misread the use of "using". My apologies. Shouldn't write comments after 20 hours in a plane :(
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 74•7 years ago
|
||
No worries. Thanks for checking.
Comment 75•7 years ago
|
||
and some performance improvements from this landing... :) == Change summary for alert #6623 (as of May 14 2017 06:20 UTC) == Improvements: 5% sessionrestore windows7-32 opt 932.25 -> 882.33 5% sessionrestore windows8-64 opt 896.69 -> 853.17 4% sessionrestore_no_auto_restore windows7-32 opt 958.54 -> 916.00 4% sessionrestore_no_auto_restore windows8-64 opt 917.44 -> 876.75 3% sessionrestore_no_auto_restore windows8-64 opt e10s707.19 -> 683.42 3% sessionrestore windows8-64 opt e10s 683.75 -> 661.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6623
Assignee | ||
Comment 76•7 years ago
|
||
\o/
Comment 77•7 years ago
|
||
looking at autophone alerts, there was a range where we couldn't build: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9730f88bdbd3d167d3c5b1acb0618cd2533bb570&tochange=9795dacd97e1dafdfc23ad1f2188cca3c2b9f45e but the other commits don't look as suspect for startup/pageload improvement on physical android devices- more wins! == Change summary for alert #6611 (as of May 14 2017 16:06 UTC) == Improvements: 4% remote-blank summary android-7-1-armv8-api15 opt 907.95 -> 867.63 4% remote-blank summary android-6-0-armv8-api15 opt 853.34 -> 821.07 4% remote-twitter summary android-7-1-armv8-api15 opt 1,012.18 -> 975.24 3% remote-nytimes summary android-4-2-armv7-api15 opt 3,286.69 -> 3,183.27 3% remote-twitter summary android-6-0-armv8-api15 opt 932.24 -> 904.89 3% remote-blank summary android-4-2-armv7-api15 opt 2,070.11 -> 2,010.41 2% remote-twitter summary android-4-2-armv7-api15 opt 2,471.78 -> 2,417.32 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6611
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•