save 8 bytes per wrappable instance

[anonrig]

No description provided.

[ask-bonk]

Good — wrappable.wrapper is only accessed from setup.c++ (external to the class), and these are the only two places. All other accesses are from within Wrappable member functions using the unqualified wrapper.

Now let me assess the semantic differences:

1. KJ_ASSERT_NONNULL → KJ_DASSERT: In setup.c++:315 and wrappable.c++:322, the original KJ_ASSERT_NONNULL(wrappable.wrapper) on a kj::Maybe would fire in release builds. The new KJ_DASSERT is debug-only. In setup.c++, the context is ResetRoot, called by V8 — if the wrapper were empty here it would be a V8 bug, so a debug-only assert is reasonable. In traceFromV8, same logic — V8 should only trace an object that has a wrapper.

2. wrapper.emplace(...) → wrapper = TracedReference<>(...) in attachWrapper: Semantically equivalent. emplace constructs in-place into the kj::Maybe, while the new code constructs a temporary and move-assigns. With TracedReference, the move assignment is essentially a pointer swap, so no performance difference.

3. KJ_REQUIRE(wrapper == kj::none) → KJ_REQUIRE(wrapper.IsEmpty()): KJ_REQUIRE fires in release, and IsEmpty() is the direct check. Equivalent severity.

The key concern I want to evaluate is the KJ_ASSERT_NONNULL → KJ_DASSERT downgrade in the two traceFromV8/ResetRoot paths. These are V8 callbacks where the invariant is that the wrapper must exist — they're only called by V8 when it knows there's a traced reference. In production, if this invariant were violated, calling Trace() on an empty reference or Reset() on an empty reference would be no-ops (V8 handles these gracefully), so there wouldn't be a crash — just silent incorrect behavior. Given that KJ_ASSERT_NONNULL on a kj::Maybe was the only option before (you had to unwrap it to use it), and now the type doesn't need unwrapping, KJ_DASSERT is a reasonable choice. This is minor.

The change looks clean and correct. The optimization saves 8 bytes per Wrappable instance, which is meaningful given every JS-visible C++ object inherits from it. All the conversions from kj::Maybe<TracedReference> patterns to direct TracedReference with IsEmpty() are mechanically correct.

LGTM

github run

[kentonv]

This makes the code less type-safe.

Harris recently implemented "niche value optimization" for kj::Maybe, which is basically a generalization of what you are doing here. By implementing MaybeTraits<v8::TracedReference<T>> you can apply this saving to all instances of kj::Maybe<v8::TracedReference<T>> across the codebase, without losing any safety. Want to do that instead?

[anonrig]

@kentonv Nice! I'll update this PR.

[jasnell]

MaybeTraits makes me happy.

LGTM once Harris signs off.