tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Public fields refinement proposal with prototype-based WeakMap semantic #174

Closed trotyl closed 5 years ago

trotyl commented 6 years ago

As discussed in many threads, [[Define]] is the semantically right behavior, but current define-on-instance approach having several issues regarding inheritance.


Here's a proposal with define-on-prototype with WeakMap semantic on this which could avoids those problem.

For a class field:

class Foo {
  bar = 42
}

Would be transformed to:

const Foo_fields_bar = new WeakMap()

class Foo {
  constructor() {
    Foo_fields_bar.set(this, 42)
  }
}

Object.defineProperty(Foo.prototype, 'bar', {
  set(value) { Foo_fields_bar.set(this, value) },
  get() { return Foo_fields_bar.get(this) },
  enumerable: true,
  configurable: true,
})

This would preserve the memory-allocation per instance:

const foo = new Foo()
console.log(foo.bar) // 42
foo.bar = 84
console.log(foo.bar) // 84

In the meantime, it makes inheritance expectable. Consider existing issues:

Issue: https://github.com/tc39/proposal-class-public-fields/issues/16#issue-112348925 Demo: https://codesandbox.io/s/0qw90xpq3w Code:

class X {
  a = 1;
}

class Y extends X {
  a() { }
}

typeof (new Y).a; // "function"

Fixes the current behavior, now class methods on subclass is able to override class fields on base (the base class may not like it, but it's up to the derived class to decide).

Issue: https://github.com/tc39/proposal-class-public-fields/issues/16#issue-112348925 Demo: https://codesandbox.io/s/jjyrr065mw Code:

class X {
  get a() { return "abc" }
}

class Y extends X {
  a = 1;
}

(new Y);

Keeps current behavior that a subclass field will override base accessor.

Issue: https://github.com/tc39/proposal-class-public-fields/issues/38#issuecomment-212733187 Demo: https://codesandbox.io/s/x72o9zjrlo Code:

class SomeThing {
    someThing = {
        prop: null,
    };

    constructor(options){
        this.someThing.prop = options.prop;
    }
}

Keeps current behavior that different instance won't share fields.

Issue: https://github.com/tc39/proposal-class-public-fields/issues/57#issue-203841068 Demo: https://codesandbox.io/s/30mjyl367p Code:

class Base {
    foo = 4;
}
class Child extends Base {
    foo;
}

let c = new Child();
c.foo; //undefined

Keeps current behavior that uninitialized field would override base fields with undefined.

Issue: https://github.com/tc39/proposal-class-fields/issues/151#issuecomment-431130535 Demo: https://codesandbox.io/s/6z0jr0wl5n Code:

class Parent extends Component {
  // this is a reactive "data" property
  foo = 1

  // this is a computed property with no setter
  get bar() {
    return this.foo
  }
}

class Child extends Parent {
  bar = 2

  someMethod() {
    console.log(this.bar) // should this be 1 or 2?
  }
}

Keeps the current behavior that subclass field could override base accessor.

Issue: https://github.com/tc39/proposal-class-fields/issues/151#issuecomment-437646527 Demo: https://codesandbox.io/s/7w34x852oq Code:

class Base {
  x = 1;
}

class Sub extends Base {
  get x() { return 2; }
  set x(value) { console.log(value); }
}

const obj = new Sub(); // prints nothing
obj.x; // 2

Fixes current behavior, now subclass accessor can override base class fields.

ljharb commented 6 years ago

This would make the public fields be inherited accessor properties, instead of own data properties. That would break, for example, Object.keys/values/entries/assign. Separately, getters being non-enumerable means that public fields wouldn’t show up in for..in or other enumeration methods.

trotyl commented 6 years ago

Separately, getters being non-enumerable means that public fields wouldn’t show up in for..in or other enumeration methods.

@ljharb I have made it enumerable in the example:

Object.defineProperty(Foo.prototype, 'bar', {
  set(value) { Foo_fields_bar.set(this, value) },
  get() { return Foo_fields_bar.get(this) },
  enumerable: true, // <-- this
  configurable: true,
})

It should always behave same as current class getter/setter.

ljharb commented 6 years ago

Apologies, I’ve updated my comment. Not being an own property then, remains the problem.

trotyl commented 6 years ago

@ljharb Thanks for point that out, I haven't considered that.

But given class is designed to have a static shape, also this is the existing behavior of class getter/setter property, then any library/framework rely on property enumeration already doesn't support getter/setter usage. So I think it could be a minor issue than inheritance, which is a designed use case for class.

ljharb commented 6 years ago

It’s not designed to have a static shape - you have always been able to, and always will be able to, change the shape however you like in the constructor.

slikts commented 6 years ago

But given class is designed to have a static shape …

This isn't Java. 😉

trotyl commented 6 years ago

you have always been able to, and always will be able to, change the shape however you like in the constructor.

It's true that one can always change the shape arbitrarily in constructor, but what class fields contributes to are only the static parts of that (even with computed property name it's still static in the lifecycle), so the use case of dynamic property enumeration should not have much overlap with class fields usage.

ljharb commented 6 years ago

@trotyl the names are part of the eventual instance shape, sure, but you can’t get a full picture of the instance from just the prototype - that’s not a guarantee classes have in JS. As for dynamic property enumeration, there are plenty of overlapping use cases - even merely “i don’t want to type out the list of static names both inside and outside the class declaration” is sufficient.

littledan commented 5 years ago

I believe @zenparsing suggested something along these lines way back when. Two concerns:

hax commented 5 years ago

@littledan

Accessors may be slower than property access, especially on startup before the JIT kicks in

It's a declaration so engines can proactively optimize it.

Own properties may be sending as more JavaScript-y

We are forced to use own properties to store states because we never have any other ergonomic ways. So saying it is more "javascript-y" is logically meaningless.

And it's very "double standards" to criticize getter/setter is not "javascript-y", but all:

...could be "javascript-y".

littledan commented 5 years ago

It's a declaration so engines can proactively optimize it.

These sorts of non-local optimizations can be hard to implement before type profiling kicks in, given how dynamic a language JavaScript is. The assessment about what's difficult to optimize was based on talking with the engine implementers who make these optimizations work in practice.

...could be "javascript-y".

Well, touche. Not sure if this helps, but I see a sort of spectrum of possible semantics, which you could coarsely call "dynamic vs reliable", with [[Set]] on one side, and accessors for internal slots on the other side, and [[Define]] in the middle. Private fields and methods solidly choose the "reliable", whereas public fields landed in the middle, in partial deference to existing patterns. You could think of dynamic as "JavaScript-y"

hax commented 5 years ago

These sorts of non-local optimizations can be hard to implement

Hard to implement != impossible to implement.

I don't believe it will be harder than all past optimization engines did.

before type profiling kicks in, given how dynamic a language JavaScript is.

This is class definitions! Most time it's just static. Engines can proactively optimize it, if some bad dynamic thing happen just deoptimize it.

The assessment about what's difficult to optimize was based on talking with the engine implementers who make these optimizations work in practice.

I heard something different from other engine guys. 🤣

After all, at least JIT just works. Currently it may be a little slow before profiling kicks, so what? Does any programmers do not use getter/setter in the cases where getter/setter is suitable?

We in long time did not have good optimization on ES6 features, but we still forward to ES6 and programmers are like to use ES6 features in most cases.

So please do not worry about that. I bet you $10000 90%+ js programmers will very happy use such a little temporarily performance loss to exchange no pain of [[Define]] vs [[Set]].

littledan commented 5 years ago

I heard something different from other engine guys.

Well, there's no need to be ambiguous here; this was from a discussion that I had with @verwaest. I'd like to collect as much implementation feedback as possible to make sure we're not specifying un-optimizable features. There's a lot of effort that V8 is putting into startup performance, and I'd like to not make it worse in this case (for the same reason, I'm working on restrictions to decorators). It's also going to be important to retain the buy-in of engine implementers to continue evolving the language. cc @gsathya

We in long time did not have good optimization on ES6 features, but we still forward to ES6 and programmers are like to use ES6 features in most cases.

Many ES6 features remain very difficult to optimize, even if some of them have improved.

hax commented 5 years ago

I think I already make my point clear. Whenever accessors are suitable, programmers will use it anyway. And we know JIT works for accessors, that's enough for 90%+ programmers, 90%+ cases. If we can get startup much faster, great! Thank you, engine engineer heros! If not, ok, programmers will not blame you.

On the other hand, the pain of [[Define]] [[Set]] is very huge, most programmers are just shocked and have no idea why TC39 send them such dilemma.

verwaest commented 5 years ago

Thanks Dan.

Sorry, but I think you misunderstand JS performance at least in the browser. Do definitely use abstractions that make sense in the places where they make sense for your code, but the JIT even isn't involved in a lot of performance critical scenarios. Defaulting to something much slower just for convenience of small pieces of code where you can just it yourself with almost no cost is just a bad idea if you want the web to be fast.

The sufficiently smart compiler is a myth we really need to get rid of if we care about load-time and other types of "warmup" (e.g., latency).

On Thu, 13 Dec 2018, 18:38 HE Shi-Jun <notifications@github.com wrote:

I think I already make my point clear. Whenever accessors are suitable, programmers will use it anyway. And we know JIT works for accessors, that's enough for 90%+ programmers, 90%+ cases. If we can get startup much faster, great! Thank you, engine engineer heros! If not, ok, programmers will not blame you.

On the other hand, the pain of [[Define]] [[Set]] is very huge, most programmers are just shocked and have no idea why TC39 send them such dilemma.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-class-fields/issues/174#issuecomment-447055798, or mute the thread https://github.com/notifications/unsubscribe-auth/ArA8RvXLN_oYeJZhM2froNNS1niAWA0aks5u4pB9gaJpZM4YtLrd .

mbrowne commented 5 years ago

@hax

On the other hand, the pain of [[Define]] [[Set]] is very huge, most programmers are just shocked and have no idea why TC39 send them such dilemma.

Maybe this is a translation thing, but "very huge" implies something much more consequential than Set vs. Define. I agree that it's a very significant and serious problem (well, much more so in the case of Define), but the impact on developers will be on a pretty small scale; there's only one relatively infrequent use cases where it comes up (redeclaring the same property on a subclass). Yes I know it could be a source of insidious bugs that could bite you years later, and that's very concerning, but let's try to have a little perspective. It's not an earth-shattering disaster.

hax commented 5 years ago

Sorry, but I think you misunderstand JS performance at least in the browser.

Hope you can teach me.

Do definitely use abstractions that make sense in the places where they make sense for your code, but the JIT even isn't involved in a lot of performance critical scenarios.

Don't understand it. Are you saying JIT can't optimize accessors even it been called frequently?

Defaulting to something much slower just for convenience of small pieces of code where you can just it yourself with almost no cost is just a bad idea if you want the web to be fast.

Don't understand it. Do you mean we should drop JS and always write C++ and compile to wasm so we are not "defaulting to something much slower just for convenience"?

The sufficiently smart compiler is a myth we really need to get rid of if we care about load-time and other types of "warmup" (e.g., latency).

So use wasm?

verwaest commented 5 years ago

On Thu, 13 Dec 2018, 19:37 HE Shi-Jun <notifications@github.com wrote:

Sorry, but I think you misunderstand JS performance at least in the browser.

Hope you can teach me.

Do definitely use abstractions that make sense in the places where they make sense for your code, but the JIT even isn't involved in a lot of performance critical scenarios.

Don't understand it. Are you saying JIT can't optimize accessors even it been called frequently?

It's easy for the JIT. The JIT just doesn't even kick in yet during a lot of performance-crucial phases.

And for example an interpreter is counter-intuitively better for startup performance of more code than a simple compiler to native code. You want the combination of low-latency startup with option to tier up to great stable peak performance.

The same is true for eagerly making complex assumptions. It's a bad idea for startup, bit crucial for peak performance.

Defaulting to something much slower just for convenience of small pieces of

code where you can just it yourself with almost no cost is just a bad idea if you want the web to be fast.

Don't understand it. Do you mean we should drop JS and always write C++ and compile to wasm so we are not "defaulting to something much slower just for convenience"?

Trolling++. Always very helpful.

hax commented 5 years ago

@verwaest Ok, let's ask a very simple question.

If we make

class Test {
  x = 0
}

using

class Test {
  #x = 0
  get x() {return this.#x}
  set x(v) {this.#x = v}
}

semantic.

How bad performance could it be compare to the current own property semantic?

5%? 10%? 50%?

verwaest commented 5 years ago

It's somewhere between 2x and 6x slower depending on the VM.

On Fri, Dec 14, 2018 at 6:28 AM HE Shi-Jun notifications@github.com wrote:

@verwaest https://github.com/verwaest Ok, let's ask a very simple question.

If we make

class Test { x = 0 }

using

class Test {

x = 0

get x() {return this.#x} set x(v) {this.#x = v} }

semantic.

How bad performance could it be compare to the current own property semantic?

5%? 10%? 50%?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-class-fields/issues/174#issuecomment-447218169, or mute the thread https://github.com/notifications/unsubscribe-auth/ArA8RkLJuqIuTZMmRiPJChfrEuN_uPUGks5u4zb_gaJpZM4YtLrd .

hax commented 5 years ago

@verwaest Do you mean with JIT or without JIT? I guess the latter?

littledan commented 5 years ago

@hax We're discussing startup performance, before the JIT kicks in.

hax commented 5 years ago

@littledan So is that mean calling getter/setter 2x~6x slower before JIT kicks in? Or mean something else (like class initialize time?)

littledan commented 5 years ago

So is that mean calling getter/setter 2x~6x slower before JIT kicks in?

Yes

verwaest commented 5 years ago

That; and if it's metamorphic. JITs bail out if code isn't regular enough. Hence lots of library and framework code at least will always have that performance difference even with JIT.

On Fri, 14 Dec 2018, 15:48 Daniel Ehrenberg <notifications@github.com wrote:

So is that mean calling getter/setter 2x~6x slower before JIT kicks in?

Yes

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-class-fields/issues/174#issuecomment-447346841, or mute the thread https://github.com/notifications/unsubscribe-auth/ArA8RtrJ8BB-zN2nqxiOUUaTS8mwxWRUks5u47o2gaJpZM4YtLrd .

littledan commented 5 years ago

OK, between @ljharb 's point about Object.keys and @verwaest 's point about performance, I think we can conclude that we're not taking this suggestion for semantics (which was under discussion a few years ago, earlier in the development process of public fields).