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

Optimize the update of a has-many relationship. #7090

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Optimize the update of a has-many relationship. #7090

merged 1 commit into from
Jul 2, 2020

Conversation

pieter-v
Copy link
Contributor

Analyzing a performance issue in our application, I noticed that updating an existing relationship (with a large number of records) is very expensive. A lot of time is consumed in Array.splice.
By changing the order: first remove all records in reverse order and then add all records in correct order, is a quick win.
See output of test application that adds each time 200 records to the same relationship until 20.000
Test application can be found ember-has-many-update

Before
before

After
after

@btecu
Copy link
Contributor

btecu commented Mar 13, 2020

You can also remove the setForArray function.

@pieter-v
Copy link
Contributor Author

@btecu done ;-)

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a great idea! I kept thinking that this might result in slightly different behaviour but then I contradicted myself. Nice job. 👍

@runspired
Copy link
Contributor

Can you port the test app to being a route in our relationship performance test app?

@pieter-v
Copy link
Contributor Author

I'm looking into it

@pieter-v
Copy link
Contributor Author

@runspired I just could not figure out how to automate the access to a second route. I could of course chain it to the logic of the existing application route, but then things will quick get messy if more scenarios are added.

@runspired
Copy link
Contributor

@pieter-v don’t worry about automating the test itself I can add that part fairly easily :)

@pieter-v
Copy link
Contributor Author

@runspired I've updated the relationship performance test app.
I also added 'performance.measure' lines, so that they show up in the Performance Tab - Timings section of Chrome

@pieter-v
Copy link
Contributor Author

I further analysed the update of a relationship, and noticed that a lot of time is spend in OrderedSet.delete and more precisely in list.indexOf(obj). Since in this scenario the position of the object is known, I added a OrderedSet.deleteWithIndex (similar to OrderedSet.addWithIndex)
deleteWithIndex

@igorT
Copy link
Member

igorT commented Jun 30, 2020

@runspired are we good to go here?

@runspired
Copy link
Contributor

runspired commented Jul 1, 2020

@igorT all we need to do is rebase so that the new performance test is run so that we can see the output from this change, I am otherwise 👍 on merging this.

@pieter-v
Copy link
Contributor Author

pieter-v commented Jul 2, 2020

@runspired @igorT rebase done

@runspired
Copy link
Contributor

  • Materialization sees a 26ms improvement in setup prior to materialization
  • Add Children sees a 3847ms improvement, half in initial setup and half when pushing the update
  • unload sees a 2755ms improvement in early setup phases (prior to unload)
  • destroy sees as 2500ms improvement in early setup phases (prior to destroy)

@runspired runspired merged commit 5b2dfaa into emberjs:master Jul 2, 2020
@pieter-v pieter-v deleted the ember-has-many-update branch July 28, 2020 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants