969578 - Remove (public-facing) Handle API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

In the latest version of the typed objects specification, the Handle API has been removed, and instead typed objects are losing their identity and becoming "value objects". As the first part of this transition, I plan to remove the (public facing) part of the Handle API. I'll probably keep some of the internal helpers that allow handles to be reattached so that we can use them for our current PJS implementation so as to improve performance. Eventually they won't be needed at all and we can optimize allocation of typed objects.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug969578.diff

Comment 2

[Niko Matsakis [:nmatsakis]]

Actually I wound up ripping out the handle API as a whole and just using the (purely internal) TypedObjectPointer.

There are some user-visible changes here to the PJS APIs. These reflect the fact that, in the new typed object specs, there is no way to create a "pointer to an integer", essentially, and hence the out pointer for scalar values will be `undefined`.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug969578.diff

Just as before, but also remove the "gc/unattachedhandle" test case, which is no longer relevant.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8374409 [details] [diff] [review]
Bug969578.diff

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

This all makes sense, yes. Will you rename the remaining Handle*-named things later on, or just drop all this stuff entirely? It seems a bit weird to still talk about handles if, really, you mean to call them objectPointers.

::: js/src/builtin/TypedObject.js
@@ +748,5 @@
>      break;
>  
>    case JS_TYPEREPR_UNSIZED_ARRAY_KIND:
>      ThrowError(JSMSG_TYPEDOBJECT_HANDLE_TO_UNSIZED);
>    }

I didn't see this before, but it seems to me that this switch is a bit silly: it'll only throw for JS_TYPEREPR_UNSIZED_ARRAY_KIND, and do the same for (the non-existing) `default:` case as for all the others: nothing. Can you either add a `default:` case and throw an error, or replace this by a simple `if (DESCR_KIND(typeObj) === JS_TYPEREPR_UNSIZED_ARRAY_KIND)`?

@@ +750,5 @@
>    case JS_TYPEREPR_UNSIZED_ARRAY_KIND:
>      ThrowError(JSMSG_TYPEDOBJECT_HANDLE_TO_UNSIZED);
>    }
>  
> +  var handle = NewTypedHandle(typeObj);

Might as well just do `return NewTypedHandle(typedObj);`

Comment 5

[Niko Matsakis [:nmatsakis]]

So I intentionally left in the TypedHandle (C++) class because, in later patches bug 898356, I repurpose that class to be an "opaque typed object" (one that does not provide access to underlying buffer), which is what handles sort of transmuted into in the new spec.

That said, most of the Handle* named things in self-hosted code should be gone...I'll go double check to make sure I got them all, as well as other stray references to handles I guess.

(Plus I'll check out the switch -- I tend to prefer switches out of habit from Rust/C++, where you get useful warnings when you omit a case, but of course that doesn't apply to JS)

Comment 6

[Till Schneidereit [:till]]

(In reply to Niko Matsakis [:nmatsakis] from comment #5)
> So I intentionally left in the TypedHandle (C++) class because, in later
> patches bug 898356, I repurpose that class to be an "opaque typed object"
> (one that does not provide access to underlying buffer), which is what
> handles sort of transmuted into in the new spec.

Yeah, I was talking about the naming, nothing else.

> 
> That said, most of the Handle* named things in self-hosted code should be
> gone...I'll go double check to make sure I got them all, as well as other
> stray references to handles I guess.

There are some Handle* functions left in self-hosting indeed.

> 
> (Plus I'll check out the switch -- I tend to prefer switches out of habit
> from Rust/C++, where you get useful warnings when you omit a case, but of
> course that doesn't apply to JS)

Having a `default:` that also throws would work just as well for me. The way it is now, though, the switch is strictly functionally equivalent to a single `if`.

Comment 7

[Niko Matsakis [:nmatsakis]]

Oh, I see, I ...somehow failed to upload the latest version of this patch. Let me figure out what happened.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug969578.diff

This is the patch I meant to post.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8374573 [details] [diff] [review]
Bug969578.diff

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

Ah, that makes more sense :)

Comment 10

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb66930e064

Comment 11

[Niko Matsakis [:nmatsakis]]

(In reply to Till Schneidereit [:till] from comment #6)
> Having a `default:` that also throws would work just as well for me. The way
> it is now, though, the switch is strictly functionally equivalent to a
> single `if`.

Just for future reference: I generally prefer a (fully enumerated) switch even in this case, because it helps with refactoring. That is, when adding a new case, I often grep through the codebase for some older case to find all the places I need to add it. If I have a fully enumerated switch, even if that switch only decides between two outcomes, I am guaranteed to see the switch and decide which outcome I want for the new case.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bb66930e064