Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite loop with 100+ observers #742

Closed
fragsalat opened this Issue Mar 14, 2019 · 17 comments

Comments

Projects
None yet
4 participants
@fragsalat
Copy link

fragsalat commented Mar 14, 2019

I'm submitting a bug report

  • Library Version:
    2.2.0

Please tell us about your environment:

  • Operating System:
    All1|10]

  • Node Version:
    11.9.0

  • NPM Version:
    6.9.0

  • JSPM OR Webpack AND Version
    none

  • Browser:
    all

  • Language:
    all

Current behavior:
Aurelia ends in infinite loop when adding more than 100 observers for one expression.
This happens because the while connecting an expression an observer is created and an observer property name is searched. The current logic in front of creates an array with slot names _observer0-99. Later a while loop increases a counter and checks if there is a slot name unused yet. At the end it results in slotname "undefined" all the time and the infinite loop reached.

while (this[slotNames[i]]) {

Expected/desired behavior:
Aurelia allows adding more than 100 observers or throws an exception which explains why it is not allowed. In my eyes it should be up to the user to decide for that.

To be honest I've no clue why you created the logic like that but you may had some thoughts on that. I could create a PR to generate the property name on the fly and allow more than 100 observers but I'm not sure if that would be good.

Also I have to say that it raises a bug in our system preventing users to open details page with big entity.

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 14, 2019

@fkleuver has made some improvements related to this in vNext. Probably he can give some info on how to backport it.

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

Ha, I knew this was going to surface sooner or later :)

Already fixed in vNext, I have a backport PR in place here: #743

Just need to make sure CircleCI is working properly with the release script. @bigopon did you have a chance to PR that build script fix?

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 15, 2019

Yes #740

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

@fragsalat if it's not too much trouble to strip out the confidential bits, would you mind sharing a repro? Not because I need the repro per se, but it sounds like a lovely test case to have for vNext as well as vCurrent to build upon. No problem if you can't do it, but asking never hurts :)

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

@bigopon hm why are the declarations still stripped from comments then? https://github.com/aurelia/binding/blob/master/dist/aurelia-binding.d.ts

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Mar 15, 2019

I guess it's from old build?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

No that's from after your merge

@fragsalat

This comment has been minimized.

Copy link
Author

fragsalat commented Mar 15, 2019

Hey guys, first of all -> bing thank you for the fast reaction and fix :)
The provided solution still looks somehow overengineered in my eyes but I also don't know the complete context :D
@fkleuver yes I'll provide when I'm at work in like 2 Hours.
The example will be based on a custom Observer I've created which makes it easier to prepare an example. It's observing a property within an array of objects like I explain it in my blog post here https://blog.stagetwo.eu/entry/12-aurelia-computedfrom-array-of-objects/

I knowit's kind of an edge case and maybe my project is not well architected in that case to need such thing, but it is historically grown and I couldn't find a better approach now^^

@fragsalat

This comment has been minimized.

Copy link
Author

fragsalat commented Mar 15, 2019

This one should reproduce the behavior https://codesandbox.io/s/20lww2lpyj

Initial state 2 children whose can be selected.
When adding 98 more children and try to check one the infinite loop appears and the page should freeze

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

The provided solution still looks somehow overengineered in my eyes but I also don't know the complete context :D

@fragsalat While some things may look weird but in fact have very careful considerations behind them, other things may indeed simply be over engineered due to tunneling. It never hurts to have an extra pair of eyes looking at something :)

The factors at play here:

  • The addObserver function runs on almost every mutation that happens anywhere. It is one of the hottest code paths in the framework, so performance trumps everything.
  • String properties perform much better than numeric properties on class instances, therefore we need the strings.
  • Retrieving an existing string from an array is significantly faster than building one on the fly, therefore we need to pre-allocate and reuse them.
  • We could preallocate 1000 of them, but surely at some point someone will come along and report a bug due to an infinite loop with 1001 items :) thus, the lazy on-the-fly allocation with increments of 5 (could might as well be increments of 1 or 100, I just picked 5 as it felt reasonable)
@fragsalat

This comment has been minimized.

Copy link
Author

fragsalat commented Mar 15, 2019

Ok seems like there went already quite a lot of investigation in here to do such tweaks :)
Thanks for sharing, that are actually very interesting points. Would be interesting to have numbers here but lets stop it for now. I trust you guys you'll do the right thing :)

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

It's funny. I had micro benched this a few months ago because I wanted to re-validate old assumptions just like you did right now. I found quite clear indicators that numeric properties were slower on classes.

Now I wanted to be able to back up the claim I just made and did another (somewhat naive/contrived) microbench, and actually found the opposite.. http://jsben.ch/GiblB

Needless to say, on-the-fly strings are slow. But numbers are apparently even faster than preallocated strings now. That is interesting. @bigopon @EisenbergEffect sanity check on that benchmark?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 15, 2019

I forgot the version increment, anyway, results remain the same http://jsben.ch/xU96A

@fragsalat

This comment has been minimized.

Copy link
Author

fragsalat commented Mar 15, 2019

I had the same recently. Before a year I proofed that an iteration approach in our use case were much faster than an recursive approach. Before 1 month we made a similar check and got the exact opposite as well^^

When do you think this fix might be released?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Mar 18, 2019

@bigopon Have you been able to look into that type def file issue yet? Maybe just add a manual copy step or something

@fragsalat

This comment has been minimized.

Copy link
Author

fragsalat commented Mar 19, 2019

Thx for merging. Is there any plan when this will be released?

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Mar 19, 2019

It's already released :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.