Vendor org.graalvm.collections

[odenix]

Motivation:
Package org.graalvm.collections is used throughout the Pkl codebase. It is most heavily used in the Truffle interpreter, which requires putting most collection methods behind @TruffleBoundary. At the moment, this is done by wrapping collection methods with static methods declared in classes EconomicMaps and EconomicSets. However, static wrapper methods are inconvenient to use, decrease code readability, add some overhead, and are easy to forget about.

Changes:

• vendor package org.graalvm.collections into org.pkl.core.collection
  • org.graalvm.collections is licensed under: The Universal Permissive License (UPL), Version 1.0
  • vendored version: https://github.com/oracle/graal/tree/graal-23.0.3/sdk/src/org.graalvm.collections/src/org/graalvm/collections
  • add package-info.java with original license
  • annotate most public methods with @TruffleBoundary
  • add @Nullable annotations
    • switching to JSpecify will enable more accurate nullability checks
  • remove prefix tree classes (not used in Pkl)
  • make no other code changes to simplify review/updates
• inline and remove EconomicMaps/Sets wrapper methods
• replace usages of EconomicMaps.equals/hashCode with EconomicMapUtil.equals/hashCode
• fix ProjectDeps.hashCode()

Result:
Cleaner, safer, and more efficient code.

[bioball]

I am not sure we want to do this. This makes it hard to upgrade the collections library, and I'm not sure how much of a performance gain we get out of this.

and are easy to forget about

Thankfully, our CI builds (on main) will catch these errors if they are forgotten.

Curious about @holzensp and @stackoverflow's thoughts

[odenix]

Owning the collection library(s) used by a Truffle language implementation makes a lot of sense:

• it is desirable for performance reasons (can optimize for partial evaluation, etc.)
• it avoids ugly wrapper code (invocations)

org.graalvm.collections is a tiny (essentially one class, namely EconomicMapImpl) and very mature library. It hasn't had significant changes in years, except for some recent additions (LockFreePool, etc.) that aren't used in Pkl.

Vendoring EconomicMap(Impl) feels natural to me, even just to improve code readability/writability.

[odenix]

I just stumbled upon the following conversation in the GraalVM Slack, where a Truffle committer argues for owning the collection library used in the Pkl interpreter. He's talking about Paguro, but it's the same thing, except that Paguro, or its future replacement, is way more challenging to own than EconomicMap(Impl).

Christian Humer Apr 5th at 7:36 AM
Looking at the Pkl types a bit. I think RrbTree could be a good candidate to make partial evaluatable. Atm it looks as if there all just boundaries, which hinders optimizations. Do you have any experience trying this?

yeah you need to manually profile and specialize for recursions and things like that. that typically involves owning the library you are using.

By the way, his latest announcement could be huge for Pkl performance, but is years away unless Pkl changes its versioning strategy:

Christian Humer 3:21 PM
At long last the Bytecode DSL was merged and will be shipped with 24.2 as experimental.

In short, the Bytecode DSL allows you to generate bytecode interpreters from an AST-like specification.
This makes building bytecode interpreters as simple probably even simpler than building AST interpreters in Truffle.
The generated interpreters automatically come with batteries included:
[...]

[stackoverflow]

I wasn't too keen on this change initially. But the library is tiny and allows us to further improve on performance (not only partial evaluation).
Was discussing some other day with @holzensp exactly about EconomicMaps and how we could improve the performance of some specific use cases, if only we could patch the implementation.
So, I'm for it.

[holzensp]

I also see the benefits (and the low risk, due to the stability of this library). It would require a further update to NOTICE.txt, btw.

[odenix]

It would require a further update to NOTICE.txt, btw.

done

[bioball]

Sorry for dropping the thread here.

Does this PR make any PE optimizations? Or, are there any PE optimizations that we know about that we can follow up with in a future PR? If so, that sounds like a compelling reason to vendor this library.

GraalJS follows a very similar approach to us around wrapping EconomicMap methods with truffle boundaries: https://github.com/oracle/graaljs/blob/24ad4e445a96c1f08b824e30e62f642b42267b3f/graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/runtime/Boundaries.java#L97

[odenix]

There’s likely room for optimizations. Although I was tempted in a few places, I decided not to touch EconomicMap code in this PR to simplify review and updates. I believe that the cleaner code alone, and the elimination of the question “should I use a wrapper method here?”, justifies this PR.

Most of the types that GraalJS wraps are JDK types and cannot be vendored. GraalJS may not use EconomicMap as much as Pkl does.

[bioball]

I'd rather not vendor this if it just helps save a wrapper class. Do you mind going into a little more detail? What optimizations do you see that we can make here?

Also: @stackoverflow can you provide some clarity here?

Was discussing some other day with @holzensp exactly about EconomicMaps and how we could improve the performance of some specific use cases, if only we could patch the implementation.

What patches were we considering for this?

[odenix]

Simple optimizations are avoiding wrapper calls (already the case) and reducing the number of boundary calls. I don’t understand why you’re pushing back against a vendoring that is both more limited and more useful than other existing vendorings (JSON etc.). If I remember correctly, you even considered vendoring msgpack, which is probably 50x the size of this one.

[bioball]

In most cases, we vendor because we want to eliminate a dependency (e.g. gson, guava, xerxes). In other cases, we vendor because we needed to make modifications to an existing library (e.g. snake-yaml).

I'm not really sure what the usefulness of vendoring this one library is? It doesn't eliminate graal-sdk as a dependency, and eliminating these wrapper calls doesn't seem very impactful. And, this PR doesn't reduce any calls of @TruffleBoundary methods, unless I'm missing something?

FWIW: I think we do spend too much CPU time behind truffle boundaries. So, overall, I do think that we should re-examine how the object model works.

[odenix]

At a minimum, it makes the code much cleaner (i.e., easier to read, write, and maintain) and less error-prone (currently, wrapper methods are used in some places but not in others). It also enables potential optimizations, as indicated by myself, your colleagues, and Truffle folks ("you should own your language's collection impls"). The cost is small (small amount of code that is very mature and hasn't changed significantly in years).

[stackoverflow]

We eventually want to improve our EconomicMap implementation, the question here is how we go about it: vendoring or implementing our own class and stop using EconomicMapImpl.
The pros of vendoring for me is:

• Some methods of EconomicMap return an EconomicMapImpl meaning we would have to be careful to never call them.
• With vendoring we control the interface (EconomicMap) so we can add/change methods there as we see fit. This could be fixed by making the properties type the concrete implementation instead of the interface, but it's less principled.
• With vendoring we don't need to change our user code at all (other than removing the wrappers, but we'll do that regardless), just the underlying implementation. Though I agree that's not a strong argument.