rkoeninger / ShenScript

Shen for JavaScript
BSD 3-Clause "New" or "Revised" License
57 stars 4 forks source link

Hoist enclosed references #12

Closed rkoeninger closed 5 years ago

rkoeninger commented 5 years ago

The $.functions and $.symbols objects are replaced with a $.globals object whose values are Cell objects that contain properties for f (functions) and v (symbol values). eternal function has been added that gets or (adds and returns) a Cell for a particular name of function or symbol. Could be separate objects, but just kept them combined like in ShenSharp.

Fabrs are re-integrated from the attempt at statement-oriented syntax. Fabrs now carry a JS expression and a map of substitutions. Each lambda gets wrapped in an immediately-invoked lambda in order to enclose over the eternal-ized Cell objects for the global functions, symbol values and idle symbols (to prevent repeated invocation of Symbol.for) in that function scope.

rkoeninger commented 5 years ago

apparent performance improvement as of 527aae4 (hoisting & enclosing over idle symbols) :

scenario prev curr
test suite (sync) 11s 7s
test suite (async) 1m 4s 56s
web startup 1200ms 950ms

these are on my fairly fast machine, but as relative times, they look good

working on hoisting reference cells containing global functions

tizoc commented 5 years ago

Those numbers look great.

rkoeninger commented 5 years ago

At this point, Firefox startup time has only improved from 25s to 22s.

rkoeninger commented 5 years ago

Hoisting global function references has resulted in no noticeable performance benefit strangely ( a603055 )

It's also significantly increased the size of the deployable bundle (~700KB to ~1100KB)

On the other hand, Firefox startup went from 22s to 16s so I guess we're keeping this change.

tizoc commented 5 years ago

@rkoeninger what does the transformation entail? (sorry, I don't understand what is going on in that commit you linked).

rkoeninger commented 5 years ago

Replacing reference Cell objects with a 2-element array had no effect on performance so I'm reverting that.

tizoc commented 5 years ago

Ahhh, ok, but it is just a matter of representation, right? Not what you described here to be able to reference the functions locally (with an indirection to allow redefinitions, but still locally): https://github.com/rkoeninger/ShenScript/issues/7#issuecomment-526446503

rkoeninger commented 5 years ago

Yeah, that last comment about Cell vs array was just how the container that holds onto the optional function pointer and optional symbol value are stored. Accessing an array index might have been a little faster than a named object member, but with small objects with consistent structure, I don't think it makes a difference. JS engines are good at optimizing objects as long as their keyset doesn't change.

rkoeninger commented 5 years ago

More cleanup to do before merging, but I think this is as good as it gets for the hoisting & enclosure concept.

I was having trouble when trying to hoist & enclose idle symbols that started with capital letters, at least in some instances, not sure why. Like $.t($.l(F => F(s`X`))) being converted to (X$sym$ => $.l(F => F(X$sym$)))(s`X`). It wasn't a problem until testing more involved defprologs and datatypes.

rkoeninger commented 5 years ago

@tizoc Updated PR description if that answers any other questions

tizoc commented 5 years ago

Accessing an array index might have been a little faster than a named object member, but with small objects with consistent structure, I don't think it makes a difference.

Yes, that was the conclusion I got after doing some tests, Array and fixed-structure object were the same

rkoeninger commented 5 years ago

Might refine this more over time, but it's working so I'm merging.