1155793 - Add a way to have DOM methods that are not allowed to be moved but are allowed to be DCEd

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we conflate these two things, which is not necessarily correct going forward.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it possible to safely change the number of bits in the slotIndex field in jitinfo

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Split apart the concepts of movability and eliminatability in jitinfo, since some things are not movable but are eliminatable

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8594091 [details] [diff] [review]
part 1.  Make it possible to safely change the number of bits in the slotIndex field in jitinfo

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

This seems fine, modulo the minor confusion about alignment in Codegen.py.

::: dom/bindings/Codegen.py
@@ +8261,4 @@
>                  """,
>                  argTypesDecl=argTypesDecl,
>                  infoName=infoName,
> +                jitInfo=indent(jitInfoInitializer(True)),

nit: why is it important to indent this string and dedent it in the template?

::: js/src/jsfriendapi.h
@@ +2398,3 @@
>  };
>  
>  static_assert(sizeof(JSJitInfo) == (sizeof(void*) + 2 * sizeof(uint32_t)),

hah! I was just gonna say "we need an assert to say we don't increase the total size." Luckily, Nathan is a wise man, and left it here many moons ago.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8594094 [details] [diff] [review]
part 2.  Split apart the concepts of movability and eliminatability in jitinfo, since some things are not movable but are eliminatable

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

r=me with comment enhanced to handle reviewer confusion.

::: dom/bindings/Codegen.py
@@ +8406,5 @@
> +        affects = self.member.affects
> +        dependsOn = self.member.dependsOn
> +        assert affects in IDLInterfaceMember.AffectsValues
> +        assert dependsOn in IDLInterfaceMember.DependsOnValues
> +        return affects == "Nothing" and dependsOn != "Everything"

I'm not sure I follow this comment. Why can't we get that in the WebIDL? I agree we struggle to encode it n the jitinfo, but this bit gets encoded on this side, no?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> nit: why is it important to indent this string and dedent it in the template?

Just to make the generated code look the way handwritten code would.  The old code was kinda whitespace-ugly: only the first line of the JSJitInfo inside JSTypedMethodJitInfo was properly indented.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Why can't we get that in the WebIDL? 

We can.  The problem is that jitinfo can only express 3 different alias sets: None, Load(DOMProperty), and Store(Any) (in the terms the JIT thinks in).  So when we have DependsOn=Everything that ends up being encoded as Store(Any), which means it looks just like we have Affects=Everything.  This is basically what bug 1155796 is about.

I'll clarify the comment.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b866119def
https://hg.mozilla.org/integration/mozilla-inbound/rev/c503b0418516

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f8b866119def
https://hg.mozilla.org/mozilla-central/rev/c503b0418516