Closed
Bug 1097507
Opened 10 years ago
Closed 10 years ago
Make libxul independent of libdmd when dmd is enabled
Categories
(Core :: DMD, defect)
Core
DMD
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 3 obsolete files)
26.72 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8521193 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
Comment on attachment 8521193 [details] [diff] [review] Make libxul independent of libdmd when DMD is enabled Review of attachment 8521193 [details] [diff] [review]: ----------------------------------------------------------------- I'm lacking sufficient context to review this. Why is this useful? What exactly is it doing? ::: memory/build/replace_malloc_end_point.h @@ +47,5 @@ > struct ReplaceMallocEndPointImpl > { > + ReplaceMallocEndPointImpl() : mVersion(1) {} > + > + virtual mozilla::dmd::DMDImpl* GetDMDImpl() { return nullptr; } Is it worth commenting that this was added in version 1?
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #2) > Comment on attachment 8521193 [details] [diff] [review] > Make libxul independent of libdmd when DMD is enabled > > Review of attachment 8521193 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm lacking sufficient context to review this. Why is this useful? What > exactly is it doing? Let me answer this with an updated patch, actually.
Assignee | ||
Updated•10 years ago
|
Attachment #8521193 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 4•10 years ago
|
||
This also effectively changes how DMD is enabled from requiring both replace-malloc initialization and the DMD environment variable to requiring only the former. The DMD environment variable can still be used to specify options, but not to disable entirely. This however doesn't touch all the parts that do enable DMD by setting the DMD environment variable to 1, so the code to handle this value is kept. The MOZ_DMD_CALL business could be replaced by wrapper functions (but StatusMsg would need some va_arg). It might be preferable to go with wrapper functions, please tell me which you'd prefer.
Attachment #8521798 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•10 years ago
|
Attachment #8521193 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
I want to see this again with "end point" renamed as "bridge", but here's some more comments in the meantime. The big comment on ReplaceMallocBridgeImpl should make it clearer that it holds all the methods for every replace-malloc library in the tree. In GetDMD(): is the singleton null check necessary? I'd like various DMD names to include "Bridge", to match "ReplaceMallocBridge". E.g.: - DMD -> DMDBridge - DMDImpl -> DMDBridgeImpl - GetDMD() -> GetDMDBridge() - sDmd -> sDMDBridge Wait, there's already a DMDEndPoint/DMDBridge... that's confusing. It's a subclass of ReplaceMallocBridgeImpl, so merge it with DMDBridgeImpl? (In general, there's a *lot* of plumbing here and I'm finding it hard to keep it all straight in my head.) Are mozilla::dmd::DMD::get() and ReplaceMallocBridge::GetDMD() equivalent? Are both needed? DMD has a DMDImpl singleton. It's declared in DMD.h, but defined in XPCOMInit.cpp. That seems weird. And could the Init() call in XPCOMInit.cpp be avoided by making the singleton getter initialize it if hasn't already been initialized?
Updated•10 years ago
|
Attachment #8521798 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 6•10 years ago
|
||
This also effectively changes how DMD is enabled from requiring both replace-malloc initialization and the DMD environment variable to requiring only the former. The DMD environment variable can still be used to specify options, but not to disable entirely. This however doesn't touch all the parts that do enable DMD by setting the DMD environment variable to 1, so the code to handle this value is kept. This version changes a lot of details, and I think makes things clearer. See comments for some of the gory details involved.
Attachment #8522556 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•10 years ago
|
Attachment #8521798 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8522556 [details] [diff] [review] Make libxul independent of libdmd when DMD is enabled Review of attachment 8522556 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/replace/dmd/DMD.h @@ +95,5 @@ > + * and then for the DMD bridge. > + * Initializing the DMD bridge singleton on the first access makes the > + * overhead even worse. Either get() is inlined and massive, or it isn't > + * and a simple value check becomes a function call. */ > + static DMDBridge* get() { sSingleton.get(); } Err, missing a return here, due to my fiddling to write the comment above.
Assignee | ||
Comment 8•10 years ago
|
||
Note we could choose not to avoid the static initializer for the dmd bridge.
Comment 9•10 years ago
|
||
Comment on attachment 8522556 [details] [diff] [review] Make libxul independent of libdmd when DMD is enabled Review of attachment 8522556 [details] [diff] [review]: ----------------------------------------------------------------- As per IRC, renaming DMDBridge as DMDFuncs, and ReplaceMallocDMDBridge as DMDBridge may make things clearer. The idea being that the "bridge" is the mechanism that lets the program and the replace-malloc lib communicate, and DMDFuncs is what gets transferred over the bridge. So GetDMDBridge() would become GetDMDFuncs(), its return type would change accordingly, and the function itself would be "the bridge", or at least part of it. At least, that seems like a good idea right now but I'm not sure if it makes sense across the whole patch and whether it would genuinely improve things. So use your judgment, but having both DMDBridge and ReplaceMallocDMDBridge is confusing. Otherwise, my comments are minor but I feel like I should see it again before landing just to give me one more chance to try to understand it all :) ::: memory/build/replace_malloc_bridge.h @@ +67,5 @@ > struct ReplaceMallocBridge > { > + ReplaceMallocBridge() : mVersion(1) {} > + > + /* Method from version 1 of the bridge. */ "This method was added in version 1 of the bridge." is clearer, IMO. @@ +97,5 @@ > + /* Don't call this method from performance critical code. Use > + * mozilla::dmd::DMD::get() instead, it has less overhead. */ > + static mozilla::dmd::DMDBridge* GetDMDBridge() > + { > + ReplaceMallocBridge *singleton = ReplaceMallocBridge::get(1); The '1' is magical. It'd be clearer if you had: const int minumumVersion = 1; ReplaceMallocBridge *singleton = ReplaceMallocBridge::get(minimumVersion); ::: memory/replace/dmd/DMD.cpp @@ -9,4 @@ > #include <ctype.h> > #include <errno.h> > #include <limits.h> > -#include <stdarg.h> This file still uses va_list/va_start/va_end, so I think stdarg.h is still needed. @@ +72,5 @@ > +{ > + virtual DMDBridge* GetDMDBridge() MOZ_OVERRIDE; > +}; > + > +static ReplaceMallocDMDBridge sDMDBridge; sReplaceMallocDMDBridge would be a better name, given that DMDBridge is a separate class. ::: memory/replace/dmd/DMD.h @@ +35,5 @@ > +}; > + > +/* Wrapper class for a pointer that also checks, in debug builds, it's not > + * used before being initialized. This helps simplify the necessary > + * static initialization of DMDBridge's static member variable. */ Nit: I use // comments every where in DMD.h and DMD.cpp. Can you please do likewise? @@ +63,5 @@ > +}; > + > +/* See further below for a description of each method. The DMD bridge should > + * contain a virtual method for each of them (except IsRunning, which can be > + * inferred from the bridge existing) */ Nit: Missing '.' at end of sentence. @@ +83,5 @@ > + > + virtual void ClearBlocks(); > + > +#ifndef REPLACE_MALLOC_IMPL > + /* Using ReplaceMalloc::GetDMDBridge directly here makes: I found this unclear. Saying something like "We deliberately don't use ReplaceMalloc::GetDMDBridge here, because if we did the following bad things would happen:" would be clearer. @@ +99,5 @@ > + static DMDBridge* get() { sSingleton.get(); } > + > + /* Avoid using a static initializer by having an explicit initialization > + * call. In Gecko, it is done in NS_InitXPCOM2 in xpcom/build/XPCOMInit.cpp. > + * For convenience, the singleton is also defined there. */ And SmokeDMD.cpp has a definition as well. Maybe explain that "the program" must define it, and why? @@ +101,5 @@ > + /* Avoid using a static initializer by having an explicit initialization > + * call. In Gecko, it is done in NS_InitXPCOM2 in xpcom/build/XPCOMInit.cpp. > + * For convenience, the singleton is also defined there. */ > + static void Init() { sSingleton = ReplaceMalloc::GetDMDBridge(); } > +private: Nit: blank line before "private".
Attachment #8522556 -
Flags: review?(n.nethercote) → feedback+
Assignee | ||
Comment 10•10 years ago
|
||
I can see that singleton can be a confusing term, shoot if you have a better idea.
Attachment #8522696 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•10 years ago
|
Attachment #8522556 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
Comment on attachment 8522696 [details] [diff] [review] Make libxul independent of libdmd when DMD is enabled Review of attachment 8522696 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. This looks a lot simpler and clearer than the original version. Hooray! ::: memory/replace/dmd/DMD.cpp @@ +63,5 @@ > #include "replace_malloc.h" > #undef MOZ_REPLACE_ONLY_MEMALIGN > #undef PAGE_SIZE > > +#include "DMD.h" I had this as the first #include for a reason -- it ensures that DMD.h doesn't have any implicit dependencies on standard headers. Please move it back :) (This idea -- that Foo.h is always the first #include in Foo.cpp -- is encoded in the SpiderMonkey style guide for this reason; see the first bullet point in https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering.) @@ +69,4 @@ > namespace mozilla { > namespace dmd { > > +class DMDBridge : public ReplaceMallocBridge, public DMDFuncs Does this class need to be a subclass of DMDFuncs? @@ +1286,5 @@ > { > StatusMsg("\n"); > StatusMsg("Bad entry in the $DMD environment variable: '%s'.\n", aArg); > StatusMsg("\n"); > + StatusMsg("$DMD shall be a whitespace-separated list of |--option=val|\n"); s/shall/must/ @@ +1329,3 @@ > > + if (!e) { > + e = "1"; This causes a "$DMD = '1'" message to be printed out immediately afterwards, which is misleading. Can you instead leave |e| as nullptr, and then do this: > if (e) { > StatusMsg("$DMD = '%s'\n", e); > } else { > StatusMsg("$DMD is undefined\n", e); > } and then make Options::Options() handle the aDMDEnvVar==nullptr case? Thanks. ::: memory/replace/dmd/DMD.h @@ +34,5 @@ > + void Clear() { memset(this, 0, sizeof(Sizes)); } > +}; > + > +// See further below for a description of each method. The DMDFuncs class > +// should contain a virtual method for each of them (except IsRunning, Extraneous space before "should". @@ +35,5 @@ > +}; > + > +// See further below for a description of each method. The DMDFuncs class > +// should contain a virtual method for each of them (except IsRunning, > +// which can be inferred from the DMDFuncs singleton existing). Is it worth having IsRunning just for consistency, so you don't have to explain why it's different? @@ +58,5 @@ > +#ifndef REPLACE_MALLOC_IMPL > + // We deliberately don't use ReplaceMalloc::GetDMDFuncs here, because if we > + // did, the following would happen: > + // - the code footprint of each call to Get() larger as GetDMDFuncs ends > + // up inlined. Super-nit: I would use a '.' after "happen" and put an upper-case letter at the start of each bullet point. The idea being that the text reads normally if you were to remove the bullet points and just have it all as normal text. @@ +67,5 @@ > + // higher because there is first a check for the replace malloc bridge > + // and then for the DMDFuncs singleton. > + // Initializing the DMDFuncs singleton on the first access makes the > + // overhead even worse. Either Get() is inlined and massive, or it isn't > + // and a simple value check becomes a function call. Extraneous space before 'a'. @@ +94,5 @@ > + }; > + > + // This singleton pointer must be defined on the program side. In Gecko, > + // this is done in xpcom/base/nsMemoryInfoDumper.cpp. > + static /* DMDFuncs:: */Singleton sSingleton; Good comment. @@ +292,1 @@ > } // namespace mozilla Heh, good catch.
Attachment #8522696 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #11) > Comment on attachment 8522696 [details] [diff] [review] > Make libxul independent of libdmd when DMD is enabled > > Review of attachment 8522696 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks. This looks a lot simpler and clearer than the original version. > Hooray! > > ::: memory/replace/dmd/DMD.cpp > @@ +63,5 @@ > > #include "replace_malloc.h" > > #undef MOZ_REPLACE_ONLY_MEMALIGN > > #undef PAGE_SIZE > > > > +#include "DMD.h" > > I had this as the first #include for a reason -- it ensures that DMD.h > doesn't have any implicit dependencies on standard headers. Please move it > back :) > > (This idea -- that Foo.h is always the first #include in Foo.cpp -- is > encoded in the SpiderMonkey style guide for this reason; see the first > bullet point in > https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#. > 23include_ordering.) Except the deal here is that this include needs to be after including replace_malloc.h. Or that REPLACE_MALLOC_IMPL is defined. > @@ +69,4 @@ > > namespace mozilla { > > namespace dmd { > > > > +class DMDBridge : public ReplaceMallocBridge, public DMDFuncs > > Does this class need to be a subclass of DMDFuncs? Not really, but then we need a separate DMDFuncs instance to return from GetDMDFuncs. > @@ +35,5 @@ > > +}; > > + > > +// See further below for a description of each method. The DMDFuncs class > > +// should contain a virtual method for each of them (except IsRunning, > > +// which can be inferred from the DMDFuncs singleton existing). > > Is it worth having IsRunning just for consistency, so you don't have to > explain why it's different? It avoids modifying calling code to do a !!DMDFuncs::Get() check itself.
Comment 13•10 years ago
|
||
> Except the deal here is that this include needs to be after including > replace_malloc.h. Or that REPLACE_MALLOC_IMPL is defined. Can you put replace_malloc.h first and DMD.h second? (And add a comment about the ordering requirement.) > > > +class DMDBridge : public ReplaceMallocBridge, public DMDFuncs > > > > Does this class need to be a subclass of DMDFuncs? > > Not really, but then we need a separate DMDFuncs instance to return from > GetDMDFuncs. Seems reasonable to me. I like the idea of having a clearer separation between DMDFuncs and DMDBridge, if you don't think it's too much of a pain.
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6bd176246d
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3c6bd176246d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•