1479937 - Control relocatability with the master FOR_EACH_ALLOCKIND table

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Steve Fink [:sfink] [:s:]]

When talking to tcambpell, I wondered why there was an explicit list of AllocKindsToRelocate instead of using the FOR_EACH_ALLOCKIND table.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Control relocatability with the master FOR_EACH_ALLOCKIND table

...and when attempting to write the patch, I found out.

So I did it anyway, partly because it was an interesting challenge, and partly to see how bad it was. After having done it... I'm not sure. Should this land, or be taken out back and shot?

I definitely find it easier to spot the types that are not compacted this way, though.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8996482 [details] [diff] [review]
Control relocatability with the master FOR_EACH_ALLOCKIND table

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

Good idea to move this to the one central table but I think we should go the way of IsNurseryAllocable() and have an array of booleans and check that in a loop over all alloc kinds.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1480233-Define-compacting-GC-support-in-AllocKin.patch

I apparently also did this. I opted to avoid the macro hack go the IsMovableAllocKind route.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8996874 [details] [diff] [review]
0001-Bug-1480233-Define-compacting-GC-support-in-AllocKin.patch

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

::: js/src/gc/AllocKind.h
@@ +38,3 @@
>  
>  #define FOR_EACH_OBJECT_ALLOCKIND(D) \
> + /* AllocKind              TraceKind     TypeName           SizedType          BGFinal Nursery Moving */ \

I prefer "Compact" to "Moving", because things are moved both during tenuring and compaction, and there are some trues here that are not tenured.

@@ +220,5 @@
>      return map[size_t(kind)];
>  }
>  
> +static inline bool
> +IsMovableKind(AllocKind kind)

IsCompactibleKind? IsCompactableKind? I can't figure out which one is more common. And they both sound kinda weird. CanCompactKind? CanCompact? IsSquishyAndWillNotShatterWhenSqueezed?

But then I'd sort of also like an IsMovableKind that returns IsNurseryAllocable || CanCompact.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1480233-Define-compacting-GC-support-in-AllocKin.patch

Replaced "moving" with "compacting" and added IsMovableKind := IsNurseryAllocable || IsCompactingKind. This coves my use-case of asserting that JitCode isn't moved.

Comment 7

[Ted Campbell [:tcampbell]]

I forgot to renumber patch.. it still as old bug number

> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1765ed67a90a
> Define compacting GC support in AllocKind.h. r=sfink

Comment 8

[Ted Campbell [:tcampbell]]

https://hg.mozilla.org/mozilla-central/rev/1765ed67a90a