This replaces all the on<EventType> read-only fields in FWT widgets with an equivalent once method.
To keep this changeset simple, I've simply replaced any direct field references in JS with a call to the method. In the future, we may want to check whether the backing field is initialized yet, to avoid needlessly coercing the EventListeners object into existence.
brianThu 9 Jun 2011
So this change would theoretically avoid creating some EventListeners objects and save a bit of memory for large widget trees. And the compromise is it a bit of extra null checking/casting for the once method.
That seems like a reasonable compromise.
BTW did you check that the Java peer classes won't actually end up allocating all the EventListeners anyways?
qualidafialThu 9 Jun 2011
I haven't optimized any java peers--they were calling the accessor methods before which worked fine with the new once methods, so I left them alone. Besides, I didn't know how to access the cache field from Java.
andyThu 9 Jun 2011
Patch looks pretty good - but needs to go alittle further. Haven't dug into the Java code - but all the JavaScript code currently invokes every accessor for every widget when it gets constructed - while it checks for things to attach to the DOM.
So to make this change worthwhile - we need to update anywhere an EventListener is checked to also include a null check on the storage field - most cases are probably for loops - so maybe just something along the lines of:
I don't think any peer code should be mucking with once storage fields, that should be internal to compiler only.
This is just UI code, so neither hard core performance nor the memory aspects widgets really matter one way or the other. It just isn't really worth optimizing.
But I do think @qualidafial changeset does make the code look a little bit cleaner using once versus private sets. So I say we make the change just for that reason.
qualidafial Wed 8 Jun 2011
Changeset
This replaces all the
on<EventType>
read-only fields in FWT widgets with an equivalentonce
method.To keep this changeset simple, I've simply replaced any direct field references in JS with a call to the method. In the future, we may want to check whether the backing field is initialized yet, to avoid needlessly coercing the
EventListeners
object into existence.brian Thu 9 Jun 2011
So this change would theoretically avoid creating some EventListeners objects and save a bit of memory for large widget trees. And the compromise is it a bit of extra null checking/casting for the once method.
That seems like a reasonable compromise.
BTW did you check that the Java peer classes won't actually end up allocating all the EventListeners anyways?
qualidafial Thu 9 Jun 2011
I haven't optimized any java peers--they were calling the accessor methods before which worked fine with the new
once
methods, so I left them alone. Besides, I didn't know how to access the cache field from Java.andy Thu 9 Jun 2011
Patch looks pretty good - but needs to go alittle further. Haven't dug into the Java code - but all the JavaScript code currently invokes every accessor for every widget when it gets constructed - while it checks for things to attach to the DOM.
So to make this change worthwhile - we need to update anywhere an EventListener is checked to also include a null check on the storage field - most cases are probably for loops - so maybe just something along the lines of:
brian Thu 9 Jun 2011
I don't think any peer code should be mucking with once storage fields, that should be internal to compiler only.
This is just UI code, so neither hard core performance nor the memory aspects widgets really matter one way or the other. It just isn't really worth optimizing.
But I do think @qualidafial changeset does make the code look a little bit cleaner using once versus private sets. So I say we make the change just for that reason.
brian Thu 9 Jun 2011
I pulled into main repo changeset