Closed Bug 1097507 Opened 6 years ago Closed 6 years ago

Make libxul independent of libdmd when dmd is enabled

Categories

(Core :: DMD, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attachment #8521193 - Flags: review?(n.nethercote)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Blocks: 1087245
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?
(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.
Attachment #8521193 - Flags: review?(n.nethercote)
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)
Attachment #8521193 - Attachment is obsolete: true
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?
Attachment #8521798 - Flags: review?(n.nethercote)
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)
Attachment #8521798 - Attachment is obsolete: true
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.
Note we could choose not to avoid the static initializer for the dmd bridge.
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+
I can see that singleton can be a confusing term, shoot if you have a better idea.
Attachment #8522696 - Flags: review?(n.nethercote)
Attachment #8522556 - Attachment is obsolete: true
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+
(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.
> 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.
Depends on: 1100851
https://hg.mozilla.org/mozilla-central/rev/3c6bd176246d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1102388
You need to log in before you can comment on or make changes to this bug.