add a helper for freeing memory from JSContext/JSRuntime

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: Benjamin, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 635148 [details] [diff] [review]
implementation with a few examples
(Reporter)

Updated

6 years ago
Attachment #635148 - Flags: review?(jwalden+bmo)
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

6 years ago
ScopedCxFreePtr/ScopedRuntimeFreePtr (using SCOPED_TEMPLATE) sounds like a good idea.
Whiteboard: [js:t]
(Reporter)

Comment 3

6 years ago
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.
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

6 years ago
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);
}
Attachment #635148 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 6

6 years ago
So, when looking for more places to use this, I discovered AutoReleasePtr in jscntx.h.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.