766800 - add a helper for freeing memory from JSContext/JSRuntime

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[:Benjamin Peterson]

Attachment: implementation with a few examples

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 635148 [details] [diff] [review]
implementation with a few examples

Review of attachment 635148 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +608,5 @@
>  };
>  SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits)
>  
> +template <class T>
> +class AutoFree

Would a ScopedCxFreePtr class be better?

Comment 2

[Luke Wagner [:luke]]

ScopedCxFreePtr/ScopedRuntimeFreePtr (using SCOPED_TEMPLATE) sounds like a good idea.

Comment 3

[:Benjamin Peterson]

The problem with reusing Scoped is it has no provision for passing context along with the resource, so it's hard to get cx/runtime there.

Comment 4

[Nicholas Nethercote [inactive]]

Is it worth adding a variant of SCOPED_TEMPLATE (from mfbt/Scoped.h) that takes a |freer| argument and implementing Scoped{Cx,Runtime}{Free,Delete}Ptr in terms of that?

It's a bit odd that we have a mix of AutoFoo and ScopedFoo types for this stuff, but oh well.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 635148 [details] [diff] [review]
implementation with a few examples

Review of attachment 635148 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +607,5 @@
>      static void release(T *ptr) { Foreground::delete_(ptr); }
>  };
>  SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits)
>  
> +template <class T>

class Freer, please -- T's fine for totally generic concepts, but this case isn't really generic at all.

@@ +608,5 @@
>  };
>  SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits)
>  
> +template <class T>
> +class AutoFree

The "Scoped" name came up on the bug that added mfbt/Scoped.h, and for some reason it was deemed better than "Auto".  I don't remember why.  The reason didn't seem valid or invalid to me, it all just looked like bikeshedding, so I studiously avoided caring.

Do we anticipate this being used both for cx->free and for rt->free?  That's the only reason I can see to have this be freer-parametrized at all.  If we do anticipate that, then the CxFreePtr bit of the change seems unwise for obvious reasons.  If we don't, this name (whether Auto or Scoped I don't care) seems fine to me.  But I'm not going to argue strenuously for anything particular.

@@ +616,5 @@
> +    T *freer;
> +    void *ptr;
> +
> +  public:
> +    AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

Put the macro param on its own line so it's visually distinct from the actual arguments, like so:

AutoFree(T *freer, void *ptr
         MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

@@ +617,5 @@
> +    void *ptr;
> +
> +  public:
> +    AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : freer(freer), ptr(ptr)

This should be indented two spaces from the 'A'/'{' column, not four.

@@ +618,5 @@
> +
> +  public:
> +    AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : freer(freer), ptr(ptr)
> +    { MOZ_GUARD_OBJECT_NOTIFIER_INIT; }

{
    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
}

@@ +619,5 @@
> +  public:
> +    AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : freer(freer), ptr(ptr)
> +    { MOZ_GUARD_OBJECT_NOTIFIER_INIT; }
> +    ~AutoFree() { freer->free_(ptr); }

~AutoFree() {
    freer->free_(ptr);
}

Comment 6

[:Benjamin Peterson]

So, when looking for more places to use this, I discovered AutoReleasePtr in jscntx.h.