1428742 - We have two ways of converting a Value to a GCCellPtr

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

While looking at bug 1428507 I noticed we have two ways of converting a JS::Value to a GCCellPtr with two different implementations:

 - GCCellPtr::GCCellPtr(const Value&)
 - Value::toGCCellPtr()

The one in Value looks like it's more efficient.  We should probably remove the other one.

Comment 1

[Venkatesh (:vpit3833)]

I like to work on this.  How can I identify which GCCellPtr calls to replace with toGCCellPtr?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Venkatesh from comment #1)
Great.

I was going to suggest removing the GCCellPtr constructor that takes a single Value argument and then fixing all the compilation errors... but then I realised that you can search for calls to a specific constructor directly:

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2JS9GCCellPtrC1ERKNS_5ValueE&redirect=false

Comment 3

[Venkatesh (:vpit3833)]

Attachment: bug-1428742-attempt-01.patch

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8973383 [details] [diff] [review]
bug-1428742-attempt-01.patch

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

Thanks for working on this.  This looks fine so far.

Can you remove the GCCellPtr constructor that takes a Value too?

I think you'll have to add a toGCCellPtr() method to the WrappedPtrOperations<JS::Value, Wrapper> specialisation in Value.h, along the same lines as the other methods there.  That allows calling this on RootedValue, HandleValue, etc.

::: js/src/vm/ProxyObject.cpp
@@ +54,4 @@
>      MOZ_ASSERT(clasp->shouldDelayMetadataBuilder());
>      MOZ_ASSERT_IF(proto.isObject(), cx->compartment() == proto.toObject()->compartment());
>      MOZ_ASSERT(clasp->hasFinalize());
> +    MOZ_ASSERT_IF(priv.isGCThing(), !JS::GCThingIsMarkedGray(priv.toGCCellPtr());

There's a missing close bracket at the end of the line.

@@ +132,4 @@
>  ProxyObject::setPrivate(const Value& priv)
>  {
>      MOZ_ASSERT_IF(IsMarkedBlack(this) && priv.isGCThing(),
> +                  !JS::GCThingIsMarkedGray(priv.toGCCellPtr());

Ditto above.

Comment 5

[nightwarriorftw]

Since no one was working on this issue since last few years. I picked up this issue as suggested by @jonco . I have made the changes in the files but won't be able to get what you are trying to say in the these lines. Sorry I am newbie to this project and still understanding how things work here.

Can you remove the GCCellPtr constructor that takes a Value too?
I think you'll have to add a toGCCellPtr() method to the
WrappedPtrOperations<JS::Value, Wrapper> specialisation in Value.h, along
the same lines as the other methods there. That allows calling this on
RootedValue, HandleValue, etc.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Aman Verma from comment #5)
Can you post your patch so far?

The comments were about removing the GCCellPtr constructor overload that takes a const Value&. This needs to be removed and existing uses of it need to be converted to use Value::toGCCellPtr.

The last comment was about adding a toGCCellPtr() method to the
WrappedPtrOperations specialisation here: https://searchfox.org/mozilla-central/source/js/public/Value.h#1127

This should be implemented in the same way as all the other methods there.

Hope this helps. If you have any questions please ping me on IRC.

Comment 7

[fffredericky]

Attachment: 1428742.patch

I did remove the constructor while replacing all uses of toGCCellPtr(), along with the check-in file js/src/jsapi-tests/testGCCellPtr.cpp. I'd like to hear some feedback as I'm pretty unfamiliar with c++ language, might run into many compilation errors.

Comment 8

[tupos9]

Attachment: Bug 1428742 - use Value::toGCCellPtr and delegating ctr of GCCellPtr. r=jonco

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1aee1f6fcd23
use Value::toGCCellPtr and delegating ctr of GCCellPtr. r=jonco

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1aee1f6fcd23