1526375 - Replace use of separate functor classes with generic lambdas where possible

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

There are a bunch of places where we use functor classes in the GC, mostly in code that works on tagged pointer types where we want to extract the wrapped pointer, cast it to its actual type and then do something on the result.

Generic lambdas result in less code and are easier to follow because the code being called is inline rather than in a separate class. We should replace functor classes with lambdas where possible.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1526375-replace-dispatch-typed

Mainly this replaces DispatchTyped functions for tagged pointer arugments with MapGCThingTyped that don't have the behaviour of calling a defaultValue() method on the functor, because that isn't present on lambdas. Instead, for types that don't always wrap a GC thing pointer, they now return a Maybe templated on the functor return value.

I rewrote RewrappedTaggedPointer as TaggedPtr and got rid of the macro expansions. I think it's clearer what's going on this way, and also it doesn't require templating on the type of the arguemt which is a pain in a lambda. This also moved to Marking-inl.h.

I also ended up making some changes to the way we update tagged pointers when we trace them or call IsMarked on them etc. I changed this to only update the value if the new value is different, because this seemed to make more sense. I can split this out into a separate patch if you like because it's really orthogonal to the main point of this bug.

Phew. This was larger than anticipated.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1526375-replace-dispatch-trace-kind-typed

Along the same lines, convert the version of DispatchTraceKindTyped that handles untyped pointer plus trace kind to MapGCThingTyped.

We can't convert the first version yet because this doesn't pass a typed argument to the functor and hence doesn't work with |[](auto t) {...}|. I discoverted that there's a C++20 feature that will allow this eventually though.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1526375-replace-other-functors

Finally, convert the other functors in Marking.cpp and remove some functors that are now unused.

Comment 4

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

Comment on attachment 9042570 [details] [diff] [review]
bug1526375-replace-dispatch-typed

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

Thanks for wading through this!

::: js/src/gc/Marking.cpp
@@ +3343,5 @@
>  template <typename T>
>  bool js::gc::IsMarkedInternal(JSRuntime* rt, T* thingp) {
> +  auto result = MapGCThingTyped(*thingp, [rt](auto t) {
> +    bool marked = IsMarkedInternal(rt, &t);
> +    return MakePair(marked, TaggedPtr<T>::wrap(t));

Do you prefer using pair to using a bool& for these three functors? I don't know whether there's a difference in how much the compiler can optimize away. At least, I think this would work

    bool marked;
    auto result = MapGCThingTyped(*thingsp, [rt, &marked](auto t) {
      marked = IsMarkedInternal(rt, &t);
      return TaggedPtr<T>::wrap(t);
    });
    ...
    return marked;

It just seems a little more readable to me than the .first()/.second() stuff.

Comment 5

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

Comment on attachment 9042581 [details] [diff] [review]
bug1526375-replace-other-functors

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

::: js/src/gc/Marking.cpp
@@ +2914,5 @@
>  // Visit all object children of the object and trace them.
>  void js::TenuringTracer::traceObject(JSObject* obj) {
> +  NativeObject* nobj = CallTraceHook([this](auto thingp) {
> +                                       this->traverse(thingp);
> +                                     },this, obj, CheckGeneration::NoChecks);

missing a space before 'this'

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

Do you prefer using pair to using a bool& for these three functors? ... It just seems a little more readable to me than the .first()/.second() stuff.

Agreed. I did it this way because I thought it would turn out simpler but on reflection it didn't.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9856d1e9c9fa
Replace DispatchTyped with MapGCThingTyped and use generic lambdas rather than separate functor classes r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18dc7069087
Replace DispatchTaceKindTyped for cell pointers with another version of MapGCThingTyped and use this where possible r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb1d7ff8fa7
Replace use of functors by marking code with generic lambdas and remove other unused functors r=sfink

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9856d1e9c9fa
https://hg.mozilla.org/mozilla-central/rev/f18dc7069087
https://hg.mozilla.org/mozilla-central/rev/2eb1d7ff8fa7