Skip to content
This repository has been archived by the owner on Apr 4, 2022. It is now read-only.

Remove MutationObserver, overload setAttribute instead #34

Merged
merged 13 commits into from
Jan 29, 2021
Merged

Conversation

hmans
Copy link
Owner

@hmans hmans commented Jan 26, 2021

I have no idea why I didn't think of this sooner. Where's the catch?

Summary:

  • Removes MutationObserver completely
  • Instead, we overload BaseElement's setAttribute method
  • We still need to apply all available attributes once when the element is connecting (as we've been doing all the time)

Pros:

  • Simplifies the code base -- one less moving part to worry about it
  • Possibly better for performance? But we don't have any good data so far
  • Attributes are forwarded to object properties immediately and not in the next JavaScript tick (not a big deal in practice, but in our tests we need to queue a lot of microtasks...)

Cons:

  • setAttribute is not triggered when the user manually modifies an attribute value in the DOM.

@hmans
Copy link
Owner Author

hmans commented Jan 26, 2021

Blocked until I have a way to do better performance tests.

@RangerMauve
Copy link

Losing the ability to edit stuff from the DOM is a big deal IMO. Being able to show folks that are new to JS and HTML how to mess around with the devtools has been really powerful in getting them accustomed to the environment. Having to change source + reload all the time doesn't flow as nicely.

If this gets changed, would it be possible to enable the old behavior behind a flag or API call?

@hmans
Copy link
Owner Author

hmans commented Jan 28, 2021

If this gets changed, would it be possible to enable the old behavior behind a flag or API call?

Yeah, I think I'd like to at least offer an observe toggle in <three-game> that opts in to the Mutation Observer. I like that feature a lot, too :)

* main:
  Refactor args parsing (fixes #40)
  Fix example
  v0.3.0-3
  Resolve that circular dependency that rollup was complaining about
  Reference other objects via CSS selectors in attributes (closes #16)
  New camera handling
  BaseElement.applyAllAttributes
  <three-rect-area-light-helper>
  Make ThreeElement.object writable
  Have ThreeGame emit a "ready" event
  Ignore the src symlink
  "...deg" transforms (#33) (fixes #31)
* main:
  Change listing
  Actually mutate attributes
  Move requestFrame to BaseElement and bind it to this
@hmans
Copy link
Owner Author

hmans commented Jan 29, 2021

I've done a bit of experimenting and light benchmarking and have some good data now.

I've created a new example that spawns 100 meshes and animates them in every frame.

I have deliberately chosen an approach where the position.x, .y and .y attributes of every of these elements are changed in every frame (100 elements * 3 attributes * up to 60 frames per second.) This is not something you would do in a real project (where, instead, you would rather mutate the Three.js scene object directly), but it did give me an opportunity to test the current implementation (MutationObserver) vs. the setAttribute thing in this PR.

Result: the setAttribute approach performs significantly better. If I said "by an order of magnitude", it would probably be an understatement. Where in this demo the MutationObserver approach is essentially prohibitively slow, you don't even see the setAttribute make any noise in the profiler.

For this reason, I will be merging this PR soon. This unfortunately means that we'll be losing the ability to have a user fiddle around in their devtools, play with attributes, and see the changes reflected in the scene immediately. A couple of thoughts:

  • I love this "feature" and would be sad to lose it.
  • At the same time, I see it as mostly a novelty. It's something fun to tell newcomers to the library, but it's nothing that you would need in any actual project. This minimizes my motivation to optimize for it.
  • However, I do like the idea of making the observing of "user fiddling" opt-in. I can't, however, make a decision about this today. I do know that I want to merge this change into main and release it with 0.3, but I want to think some more about if, how and where exactly to implement opt-in observing.

If you have thoughts on this, please voice them!

Update: just to give you an idea of how much better than using MutationObserver this is: if I bump the number of dodecahedrons in the swarm from 100 to 1000, a total of 4ms is spent in setAttribute per frame, vs. ~40ms in the microtasks queued by the MutationObserver.

@hmans hmans added enhancement New feature or request and removed blocked Issue/PR is blocked labels Jan 29, 2021
@hmans hmans added this to the 0.3 milestone Jan 29, 2021
@hmans hmans merged commit e74ca07 into main Jan 29, 2021
@hmans hmans deleted the set-attribute branch January 29, 2021 16:05
@hmans hmans restored the set-attribute branch January 29, 2021 16:11
@hmans hmans deleted the set-attribute branch January 29, 2021 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants