Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#6564 closed change (rejected)

Replace desc and extend in lib/coreUtils.js with __proto__

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Core Keywords:
Cc: kzar, sergz, agiammarchi, sebastian, greiner, fhd Blocked By:
Blocking: #6425 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29746555/

Description

Background

There are two functions in lib/coreUtils.js that are currently no longer required: desc and extend. Both of these can be replaced with __proto__ in the object literal for the prototype.

For example, where we previously wrote this:

const {extend} = require("./coreUtils");

InvalidFilter.prototype = extend(Filter, {
  type: "invalid"
});

Now we can write this:

InvalidFilter.prototype = {
  __proto__: Filter.prototype,
  type: "invalid"
};

This is twice as fast.

What to change

Remove desc and extend from lib/coreUtils.js and replaces their use everywhere else with __proto__ in the object literal for the prototype.

Change History (15)

comment:1 Changed 20 months ago by mjethani

__proto__ in object literals appears to be supported on all our platforms.

comment:2 Changed 20 months ago by mjethani

If we must keep the extend function, we can rewrite it as:

function extend(cls, properties)
{
  return Object.assign({__proto__: cls.prototype}, properties);
}

comment:3 Changed 20 months ago by mjethani

  • Review URL(s) modified (diff)

comment:4 Changed 20 months ago by kzar

  • Cc sebastian added

This one seems kind of at odds with #1985, do you have an opinion Sebastian?

comment:5 follow-up: Changed 20 months ago by agiammarchi

FWIW __proto__ is Annex B of the specifications, not a major recommendation.

However, AFAIK, if raw performance is a concern, {__proto__: value} is the fastest but obj.__proto__ = value is not recommended over setPrototypeOf (the latter grants results, the former doesn't) and same goes for obj.__proto__ vs getPrototypeOf: methods grant results, __proto__ can be a footgun (think about null objects).

comment:6 Changed 20 months ago by kzar

IMO we should clarify this in our coding style guide whatever we decide, better to get the discussion out the way once.

comment:7 Changed 20 months ago by kzar

  • Cc greiner fhd added

comment:8 in reply to: ↑ 5 ; follow-up: Changed 20 months ago by kzar

Replying to agiammarchi:

...(the latter grants results, the former doesn't)...

Sorry if this is a dumb question but what do you mean by this part? I don't understand.

comment:9 in reply to: ↑ 8 Changed 20 months ago by agiammarchi

Replying to kzar:

Sorry if this is a dumb question but what do you mean by this part? I don't understand.

The following summarizes why __proto__, if used as accessor, can be a foot gun.

// imagine a funny/unknown/external/native object like this one
const funny = JSON.parse('{"__proto__": null}');

// regular code that fails as accessor (not a getter)
funny.__proto__;                    // null
Object.getPrototypeOf(funny);       // Object.prototype

const proto = {another: 'object'};

// regular code that fails as accessor (not a setter)
funny.__proto__ = proto;
proto.isPrototypeOf(funny);         // false
funny.another;                      // undefined

Object.setPrototypeOf(funny, proto);
proto.isPrototypeOf(funny);         // true
funny.another;                      // "object"

As you can see, using Object public methods always deliver developers expectations (unless these are wrapped/modified, of course or the object is frozen and prototype won't be set).

However, using it inline as literal {__proto__: value} is always OK, so if that's the only pattern we'd like to use with __proto__ I agree it's both shorter to write and faster, as long as it's used in its literal form only.

I hope I've better explained my concerns.

Regards

Last edited 20 months ago by agiammarchi (previous) (diff)

comment:10 follow-up: Changed 20 months ago by sergz

Sorry if this a dumb questions but what about using of classes and removing of those desc and extend?

comment:11 in reply to: ↑ 10 Changed 20 months ago by agiammarchi

Replying to sergz:

Sorry if this a dumb questions but what about using of classes and removing of those desc and extend?

AFAIK all our target browsers support classes so ... if you'd ask me, I'd say of course, use classes :-)

comment:12 Changed 20 months ago by sebastian

As Dave pointed out, we deliberatly moved away from __proto__ in #1985, since it was non-standard back then. Now its standardized in ES2016 as a legacy feature which, however, doesn't make it any more appropriate to use it (in new code).

But as sergz and agiammarchi pointed out, using the new class syntax seems to be the best option. It is in compliant with the standard, not legacy, requires the least boilerplate, makes any helper functions obsolete, and should perform well.

comment:13 Changed 20 months ago by mjethani

Alright, if we agree to use classes then I'll close this. Thanks!

comment:14 Changed 20 months ago by mjethani

  • Resolution set to rejected
  • Status changed from new to closed

comment:15 Changed 20 months ago by agiammarchi

For record sake, and as heads up, this is a bug that took me a while to figure out:
https://github.com/Microsoft/ChakraCore/issues/4970

MS Edge super() call might break if the extend is a legacy function that does not explicitly return a context.

The work around needed for Edge only is to transform function Legacy() {} into function Legacy() { return this; }`

Even better, I guess, if legacy classes are converted into ES6 classes as we go/need to extend them.

edit it got fixed and it will be out with the next version of Edge
https://github.com/Microsoft/ChakraCore/pull/4670

Last edited 20 months ago by agiammarchi (previous) (diff)
Note: See TracTickets for help on using tickets.