Closed Bug 1471931 Opened 2 years ago Closed 2 years ago

Clean up malloc uses, switch to UniquePtr where possible/useful

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Switch a few call sites to use js_pod_malloc/js_pod_calloc/js_pod_realloc for more concise code (avoids repeating the type name and manual casting).
Attachment #8988524 - Flags: review?(sphink)
Hunted down a few manual js_free calls and replaced them with UniquePtr. 

Following the existing code style, the result variable for |cx->make_pod_array(...)| was changed to |auto| in a few places.
Attachment #8988534 - Flags: review?(sphink)
Change a few callers to use the more "correct" allocation function.
Attachment #8988535 - Flags: review?(sphink)
Adds the requested NewString functions which accepts UniquePtrs.

All JS internal code was updated to use the new UniquePtr based functions, the old functions are now only used for the JSAPI functions JS_NewLatin1String, JS_NewUCString, and JS_NewUCStringDontDeflate. After bug 1402247 is finished, I plan to change the JSAPI function to also only accept UniquePtr, but for now I didn't want to bitrot the existing patches in the other bug. (The JSAPI functions always use CanGC, therefore I removed the AllowGC template parameter from the old NewString functions, too.)
Attachment #8988536 - Flags: review?(sphink)
Use auto + MakeUnique instead of UniquePtr<Type> + js_new<Type> in a few places.

Also replace some manual js_delete calls with UniquePtr.

And finally also replace some |mozilla::UniquePtr<Type, JS::DeletePolicy<Type>>;| typedefs with |js::UniquePtr<Type>| which is equivalent, but shorter.
Attachment #8988542 - Flags: review?(sphink)
Attachment #8988524 - Flags: review?(sphink) → review+
Comment on attachment 8988534 [details] [diff] [review]
bug1471931-part2-more-uniqueptr.patch

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

Wow, a ton of nice cleanups fall out of this.

::: js/src/builtin/Profilers.cpp
@@ -190,5 @@
>  #ifdef MOZ_PROFILING
>  
> -struct RequiredStringArg {
> -    JSContext* mCx;
> -    char* mBytes;

Good riddance!

::: js/src/vm/Runtime.cpp
@@ +536,4 @@
>      if (!newLocale)
>          return false;
>  
> +    defaultLocale.ref() = std::move(newLocale);

I thought there was a warning if you did this? Something about using std::move prevents RVO or... something?

@@ +569,3 @@
>          *p = '-';
>  
> +    defaultLocale.ref() = std::move(lang);

Same comment as above.
Attachment #8988534 - Flags: review?(sphink) → review+
Attachment #8988535 - Flags: review?(sphink) → review+
Comment on attachment 8988536 [details] [diff] [review]
bug1471931-part4-newstring-unique.patch

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

\o/
Attachment #8988536 - Flags: review?(sphink) → review+
Comment on attachment 8988542 [details] [diff] [review]
bug1471931-part5-makeunique.patch

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

nice.
Attachment #8988542 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] (PTO June 29-July 4) from comment #6)
> ::: js/src/vm/Runtime.cpp
> @@ +536,4 @@
> >      if (!newLocale)
> >          return false;
> >  
> > +    defaultLocale.ref() = std::move(newLocale);
> 
> I thought there was a warning if you did this? Something about using
> std::move prevents RVO or... something?

Without std::move this line would be a plain assignment, which is not allowed for UniquePtr.

---
In file included from /home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/js/src/Unified_cpp_js_src37.cpp:11:
/home/andre/hg/mozilla-inbound/js/src/vm/Runtime.cpp:539:25: error: overload resolution selected
      deleted operator '='
    defaultLocale.ref() = newLocale;
    ~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
/home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/mozilla/UniquePtr.h:497:8: note: 
      candidate function has been explicitly deleted
  void operator=(const UniquePtr& aOther) = delete; // assign using std::move()!
       ^
/home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/mozilla/UniquePtr.h:445:14: note: 
      candidate function not viable: no known conversion from 'JS::UniqueChars' (aka 'UniquePtr<char
      [], JS::FreePolicy>') to 'mozilla::UniquePtr<char [], JS::FreePolicy> &&' for 1st argument
  UniquePtr& operator=(UniquePtr&& aOther)
             ^
/home/andre/hg/mozilla-inbound/js/src/build-debug-opt-obj/dist/include/mozilla/UniquePtr.h:452:14: note: 
      candidate function not viable: no known conversion from 'JS::UniqueChars' (aka 'UniquePtr<char
      [], JS::FreePolicy>') to 'decltype(nullptr)' (aka 'nullptr_t') for 1st argument
  UniquePtr& operator=(decltype(nullptr))
             ^
1 error generated.
---
Add a missing |js::| qualifier to js/src/jit/arm64/vixl/MozSimulator-vixl.cpp, otherwise no changes. Carrying r+.
Attachment #8988542 - Attachment is obsolete: true
Attachment #8988688 - Flags: review+
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd01847472fb
Part 1: Replace some js_malloc/js_calloc/js_realloc with their js_pod_malloc/js_pod_calloc/js_pod_realloc counterparts. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1fc57cce36
Part 2: Replace manual memory management with UniquePtr in a few places. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2861f3dcc58
Part 3: Switch to pod_calloc_with_extra, new_, make_pod_array, and make_zeroed_pod_array where possible. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e320fc1cbb8
Part 4: Add NewString and NewStringDontDeflate functions which accept UniquePtr. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/266767b68e8c
Part 5: Use MakeUnique in more places and replace manual js_delete with UniquePtr. r=sfink
Keywords: checkin-needed
Depends on: 1472666
You need to log in before you can comment on or make changes to this bug.