tc39 / proposal-decorators

Decorators for ES6 classes
https://arai-a.github.io/ecma262-compare/?pr=2417
2.74k stars 106 forks source link

@init: Why should it be removed from the proposal? if any decorator can always optionally return an initializer, @init: would not be necessary. #380

Closed pabloalmunia closed 2 years ago

pabloalmunia commented 3 years ago

First of all, I apologize for my English. It is pretty poor, and I may not be able to express my ideas very clearly.

After several days of building the experimental transpiler, I have a clearer picture of some fundamental aspects of the current decorator proposal. The most important and most impactful conviction is that it is a bad idea to include @init:.

the initialization is important

Initialization is an essential feature in the decorators environment. Several use cases can only be solved using some initialization once the class has been entirely built and decorated.

Whether to use an initialization function is up to the decorator developer, which may or may not utilize this functionality in its internal implementation.

@init: is an overload for the decorator user

The decorator mode invoked as @init: puts in the hands of the decorator user the need to include this clause if the decorator has been implemented so that it needs an initializer function. The decorator constructor must properly document this need, and the decorator user must always be aware of it. A decorator that requires an initializer will typically not be able to be used with a simple @ and should probably throw an exception if it is invoked in this way.

The need to express that a decorator needs an initializer should rest solely with the decorator developer and not need the decorator user support.

@init: is unnecessary for initialization, all decorator can return a initializer

Decorators invoked with an @init: may or may not include an initialize attribute on the object they return. Until the decorator is executed, we do not know if we will have to implement an initializer.

Identifying a decorator that requires an initializer is simple. We have to check if its return is an object instead of a function (which decorators do not need an initializer return). Even if it returns a thing, we have to check if this object has an initialize property.

This a simple helper function for process:

function __applyDecorator(result, origin, intializators) {
  if (typeof result === "undefined") {
    return origin;
  }
  if (typeof result === "function") {
    return result;
  }
  if (typeof result === "object") {
    if (typeof result.initialize === "function") {
      intializators.push(result.initialize);
    }
    return result.method || result.get || result.set || result.definition || origin;
  }
  throw new TypeError("invalid decorator return");
}

Although it may seem that the decorator execution is a very late moment to know if there is going to be an initializer or not, the truth is that it is the only moment where we know that an initializer has been returned.

@ as the only type to invoke decorator and desugars.

In our informal exercise in creating a transpiler, we have seen that we can use a simple support function so that all decorators can return:

For example, this decorator return an object with a method and an initilize:

function logged(value, { kind, name }) {
  if (kind === "init-method") {
    return {
      method(...args) {
        console.log(`starting ${name} with arguments ${args.join(", ")}`);
        const ret = value.call(this, ...args);
        console.log(`ending ${name}`);
        return ret;
      },

      initialize(initialValue) {
        console.log(`initializing ${name}`);
        return initialValue;
      }
    };
  }
}

class C {
  @logged
  m() {}
}

let c = new C();
c.m(1);

This code "desugars" to the following:

const _member_initializers = [];

class C {
  constructor() {
    _member_initializers.forEach(initialize => initialize.call(this));
  }
  m() {}
}

C.prototype.m = __applyDecorator(logged(C.prototype.m, {
  kind: "init-method",
  name: "m",
  isStatic: false,
  isPrivate: false,
  defineMetadata: __DefineMetadata(C.prototype, "m")
}), C.prototype.m, _member_initializers);

let c = new C();

c.m(1);

Advantages and disadvantages of eliminating @init:

Pro

Cons

In my opinion, the advantages outweigh the disadvantages.

Keep the constraints

Remove @init: keeps the constraints set by these three rules:

A: The "shape" of the class should be apparent syntactically, without executing code.

B: It should not be too complicated to process decorators, as this corresponds to a complex implementation.

C: Minimize or eliminate observable mutations to objects while setting up the class.

trusktr commented 3 years ago

Which referenced overhead, and is that noted somewhere else? I did not get that from the meeting notes.

AFAIK there are two: One directly due to the need to keep a registry of initializers for each class and run them in constructor

@blikblum

This registry can be added by engines only if a decorator uses addInitializer. Engines can compile classes with this code entirely missing if no decorator called the addInitializer method. I don't see the performance issue yet. Or is it hard for JS engine devs to compile or not compile features into the class?

The first is incontestable, without @init any class with a decorator would require the initializers registry even if does not use initializers. With @init, only class with @init decorators would need this registry

Based on my last paragraph, classes without decorators that called addInitializer would simply not have those code paths compiled into their implementation. It would have zero cost unless a decorator calls addInitializer, in which case the runtime cost is necessary and boils down to calling each initializer.

Other indirectly, since the initializers could potentially change the shape of instance or prototype thus preventing advanced optimization

We don't have to worry about this: everyone simply needs to know arbitrary code in a constructor can deoptimize.

addFinisher into the context is a very good idea. We have put too many options in the decorator's return.

For example as an author if you wanted to create a new decorator from two other decorators, that should effectively be as simple as:

function decoAB (value, context) {
  return decoB(decoA(value, context), context)
}

:+1::+1::+1::+1: Having only one type of return value, or no return value at all, would be much more ideal than the current situation which is a bit messy for composition.

The more we can simplify (or eliminate) the return value, the better.

I still don't think we need any return value at all. Circling back to my idea in https://github.com/tc39/proposal-decorators/issues/380#issuecomment-826361254, @nicolo-ribaudo you said

That still doesn't guarantee that it will work: you could have a decorator applied to a class in a different realm, with different built-in constructors (which is the reason why we have Array.isArray rather than just instanceof Array).

Ok, then we can just keep the passed-in POJO format as the current proposal has, relying on checking type, while still eliminating return values if the API is robust enough:

// Composition:
function myDeco(context) {
  if (context.type == 'method') {
    decoA(context)
    decoB(context)
  } else if (context.type == 'property') {
    decoC(context)
    decoD(context)
  } else {
    throw new Error('This decorator can only be used on methods or properties.')
  }
}

No return values there assuming decorators have all the necessary APIs available to do whatever they need to do. :+1::+1::+1:

Plus this way new APIs can be added later to the spec (for whatever reasons we haven't pondered yet) without complicated return values getting in the way or usage becoming more difficult. The current overloaded return values means future extension of the spec is more difficult without introducing breaking changes, and of course breaking changes are probably not an option. Adding new methods or properties allows more flexibility while staying backward compatible more easily.

Finally the context POJO could be frozen (or just some properties frozen).

Engine implementation:

If a class author uses any decorators that never add initializers, then the class will be optimized, right?

Plus! The initializers would be injected only for decorators that call the API. There's no need for conditional checking here.

Doesn't this mean we can eliminate @init? Do we really need a static @init for optimization?

@pabloalmunia We can make an PoC like I described, with your transpiler, and add it to the perf comparison. I'm not sure it would be comparable to a comparison of native implementations. Would it be?

pabloalmunia commented 3 years ago

@pabloalmunia We can make an PoC like I described, with your transpiler, and add it to the perf comparison. I'm not sure it would be comparable to a comparison of native implementations. Would it be?

In my humble opinion, it is not possible to do a PoC like the one you have described with a transpiler, and add it to a perf comparison. A transpiler cannot work with optimised or deoptimised classes directly, to achieve this you would have to use V8 hits (such as %GetOptimizationStatus and %OptimizeFunctionOnNextCall) which would make the test not very comparable.

TomorrowToday commented 3 years ago

Seems clear that @init: is a problem for many users. Certainly sounds like a pain for adding listeners/handlers and registration functions that need to preserve instance level context.

Not sure if anyone else has proposed this: instead, why not make people chasing the performance dragon use syntax like @static: to squeeze out that little gain? And If this is where they think they need to gain performance, there's a good chance they've already abandoned easily readable code anyway. So the rest of us can continue to use a standard --maybe a little slow-- @decorator and not bear the ugly syntax tax.

That all said, I'd prefer just about any syntax to nothing. So if it must be @init, I'll hold my nose and eat it, but(!) someday I'll be able to write performant Python in the browser and ditch that weird @init:decorator hahaha :P

trusktr commented 3 years ago

I still like this idea more: classes are optimized by default and do not check for a registry during construction, unless an addInitializer call happens during decoration. It seems like that would work. I'm no engine developer though.

Why does the optimization have to be statically analyzable? Is it in order to avoid first making an optimized class and then having to de-optimize it (so instead just creating a de-optimized class up front (containing the initializer registry) due to static analysis)?

TomorrowToday commented 3 years ago

It does seems a little strange to insist on making it statically analyzable when the design pattern it's based on is dynamic... But it seems there are two big advantages to the static approach:

  1. Performance cost by default is only paid once at compilation
  2. Laggard browsers can be polyfilled more easily / the syntax can be easily "desugared" by build tools
trusktr commented 3 years ago

In my humble opinion, it is not possible to do a PoC like the one you have described with a transpiler, and add it to a perf comparison. A transpiler cannot work with optimised or deoptimised classes directly,

Random idea: maybe both class definitions can be included in the output, one with the initializer registry, one without, then based on decorator execution (depending on if they call addInitializer or not) a final variable can be assigned one class or the other.

This would result in output being twice as big, etc, but we're not measuring that. For purposes of the topic, I guess we only need to measure speed of the final class that ends up being used.

trusktr commented 3 years ago

As for native implementations, I could imagine how making an optimized class, then making a deoptimized class, would have a perf hit. But if we're talking about modifying an optimized class to deoptimize it (f.e. injecting byte code into the constructor or something similar) then I think the perf hit may be negligible compared to statically making an optimized or deoptimized class up front. That's why I guess that measuring loading of duplicate classes of the transpiler output is not necessary.

Can a JS engine dev chime in on this? How accurate is this optimization speculation?

(@RReverser, @LeszekSwirski, @verwaest, @anba, @jonco3, @hotsphink, @tadeuzagallo)

LeszekSwirski commented 3 years ago

I haven't been following this discussion closely, so I might be misunderstanding the use of terminology here, but at least in V8 optimising/deoptimising JIT compilation is distinct from the sort of static optimisations (e.g. class boilerplates) which suffer when decorators are too dynamic, so I'm not sure what the question is.

Are you describing a system which assumes that decorators don't have an initializer and therefore the statically inferrable class shape is correct, but can discard that boilerplate and create classes "manually" if a decorator introduces an initializer? My first concern would be the observability of the class instance to the initializer at the moment where that assumption is violated; AFAICT the instance isn't visible to non-initializer decorators, so we may have already initialized fields which aren't yet visible and need to be rolled back. I also don't see a way of not paying some sort of penalty (performance and memory) to preserve enough information for the "deoptimized" case. I'd have to take a closer look at the proposal to comment more in-depth.

trusktr commented 2 years ago

Are you describing a system which assumes that decorators don't have an initializer and therefore the statically inferrable class shape is correct

Yes, that's what I described.

I'm no engine dev, but based on what I read in some public articles, it seems that a single "hidden class" could be made for the entire statically-known class (including any properties it inherits from statically-known classes) at class definition time. This would be in contrast to one "hidden class" per property at instance creation time. But I don't really know the inner details.

So, it seems as if classes with statically-known class fields could be significantly faster than classes-with-properties-in-constructor or function classes.

This also made me wonder if it is possible to enforce a user to write statically-known classes using a built-in decorator: https://github.com/tc39/proposal-decorators/issues/434

LeszekSwirski commented 2 years ago

What you're describing with hidden classes is roughly correct, we initialise a "boilerplate" which already has the right hidden class but is populated with uninitialized value sentinels, and we avoid transitions by simply populating these uninitialized fields. Unfortunately, this relies on us being able to statically tell that this is something that's safe to do, which means that a) no path can write other fields, and b) no path can read the uninitialized fields. This is also what makes runtime class shape changes difficult to deal with, we can no longer make static guarantees, which means we have to add runtime guards/checks, which means things get slower.

glen-84 commented 2 years ago

Should this issue be closed now? (https://github.com/tc39/proposal-decorators/pull/436)

pabloalmunia commented 2 years ago

Yes, this issue must be closed