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 finding singular target elements #266

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@javan
Copy link
Contributor

commented Aug 12, 2019

Tested (somewhat unscientifically) using the following markup nested several levels deep:

<div data-controller="x" data-target="x.a">
  <span>
    <span data-target="x.b"></span>
    <span data-target="x.b"></span>
  </span>

  <span data-target="x.c"></span>
  <span data-target="x.c"></span>

  <div data-controller="x" data-target="x.a">
    …
  </div>
</div>

Before:

this.aTarget 0.140ms
this.bTarget 0.230ms
this.cTarget 0.160ms

After:

this.aTarget 0.035ms
this.bTarget 0.140ms
this.cTarget 0.110ms
@javan javan requested a review from sstephenson Aug 12, 2019
@sstephenson

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

The code itself looks fine 👍 I’ll leave it up to you as far as merging, though.

I’d prefer not to make micro-optimizations like this without some sort of corpus we can benchmark against. As you’ve noted, the test case here is unscientific and not really representative of an actual DOM tree, and AFAIK we don’t have any reason to think the current implementation will be a performance problem. We also wouldn’t have any way to know if we were to regress in the future.

@javan javan merged commit a82a8a5 into master Sep 18, 2019
2 checks passed
2 checks passed
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@javan javan deleted the optimize-singular-targets branch Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.