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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improve context.find/findByTag() and interceptor perf #4377

Merged
merged 3 commits into from Jan 27, 2020

Conversation

@raymondfeng
Copy link
Member

raymondfeng commented Jan 7, 2020

Fixes #4356 and #4363

Matching all bindings by a filter function can be expensive. This PR improves performance for one of the primary usage - find bindings by tags.

Main changes in this PR:

  1. Make Binding to be EventEmitter - emitting events when binding scope/tags/value are changed
  2. Set up listeners in Context to react to binding events to maintain an index of bindings by tag
  3. Optimize Context.findByTag to leverage binding index if possible
  4. Change interceptor to find matching global interceptors by tag

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

Copy link
Member

bajtos left a comment

I reviewed the pull request at high level. It's nice to see that a relatively easy change can improve the performance so much.

I am not very happy about the proposed (internal) design, let's do few more iterations please to improve it.

@@ -148,7 +157,7 @@ type ValueGetter<T> = (
* Binding represents an entry in the `Context`. Each binding has a key and a
* corresponding value getter.
*/
export class Binding<T = BoundValue> {
export class Binding<T = BoundValue> extends EventEmitter {

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 9, 2020

Member

I am concerned about the performance implications of turning every binding into an event emitter. I am expecting that consumers of these events will want to listen on Context instances, not on individual bindings.

Have you considered changing this implementation to emit the events on the Context object the binding is added to? If the binding is not added to any Context object yet, then I am arguing that the events can be silently discarded.

I am also surprised that we did not need these Binding-level events when implementing Context Observer. Does it mean that Context observers were not able to be notified about these kinds of changes before? Did we perhaps use a different mean to achieve this functionality for context observers?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 9, 2020

Author Member

At this moment, the same binding instance can be added to multiple context objects. If we decide to treat each binding to be optionally owned by one context, we can emit such events on the owning context.

When we implemented ContextObserver - which is async, we choose to process bind and unbind events at a late cycle to work around the issue that bindings can be changed. Now with binding events, we can probably revisit the implementation to track binding.changed events.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 10, 2020

Author Member

IMO, setting Context ref on Binding is similar as Context registering itself as a listener on Binding from performance overhead perspective.

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 13, 2020

Member

At this moment, the same binding instance can be added to multiple context objects.

Somehow I thought that a Binding is already holding a reference to the single owning Context. I check the source code and see that it does not. As you wrote, it's possible to add the same binding to multiple contexts, therefore I agree with your originally proposed design, where each Binding is an EventEmitter 馃憤

When we implemented ContextObserver - which is async, we choose to process bind and unbind events at a late cycle to work around the issue that bindings can be changed. Now with binding events, we can probably revisit the implementation to track binding.changed events.

Make sense. I'd like to revisit this part sooner rather than later, to ensure consistency.

binding: Readonly<Binding<unknown>>,
context: Context,
event: string,
) => void;

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 9, 2020

Member

Can you please explain what's the difference between ContextObserver and ContextEventListener? When should our users use which?

Please add a tsdoc comment for this interface. I think we should update the documentation to mention this new way of observing context changes.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 9, 2020

Author Member

ContextEventListener is a sync function as typical Node.js event listeners. The ContextObserver is async handler.

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 13, 2020

Member

ContextEventListener is a sync function as typical Node.js event listeners. The ContextObserver is async handler.

Sure, I can tell this part from the source code. I am asking for user-centric documentation - when should our users choose ContextObserver and when to choose ContextEventListener? As framework authors, we should set clear guidance here.

packages/context/src/context.ts Outdated Show resolved Hide resolved
) {
return this.find(filterByTag(tagFilter));
}
return this._findByTagIndex(tagFilter);

This comment has been minimized.

Copy link
@bajtos

bajtos Jan 9, 2020

Member

While this solution works for the particular case of improving performance of controller methods when there are no interceptors configured, I am concerned it's too specialized and won't work for other scenarios. For example, if the application has 100 different interceptors bound in the context, to execute a controller method with no interceptors, we will still have to scan that array of 100 interceptors.

Also IIRC our previous conversations, we wanted to move away from findByTag in favor of more generic filtering. In this pull request, you are reversing that direction.

Have you considered implementing a more generic solution, one that will allow consumers outside of @loopback/context to implement their own cache and have an easy solution for invalidating it?

I think ideally, we want to:

  • for each controller method, cache the actual list of interceptors to invoke
  • invalidate cache entries when the controller method metadata changes or when an interceptors is bound or unbound

Maybe we can treat this idea as a long-term goal and implement tag indexing as a short-term performance improvement.

Thoughts?

/cc @strongloop/loopback-maintainers

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 9, 2020

Author Member

I have added a few more commits to fully leverage BindingTagFilter whenever it is possible - even for ContextView.

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch 5 times, most recently from d040e6b to 213642c Jan 9, 2020
@raymondfeng raymondfeng requested a review from bajtos Jan 10, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 10, 2020

@bajtos PTAL.

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch from 213642c to ab3fe0f Jan 10, 2020
@raymondfeng raymondfeng changed the title feat: improve interceptor perf feat: improve context.find() and interceptor perf Jan 10, 2020
@raymondfeng raymondfeng changed the title feat: improve context.find() and interceptor perf feat: improve context.find/findByTag() and interceptor perf Jan 10, 2020
@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch from ab3fe0f to 5fe3852 Jan 13, 2020
@raymondfeng raymondfeng requested a review from bajtos Jan 13, 2020
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 14, 2020

This pull request is introducing several changes: it introduces new Binding-level events, adds ContextEventListener, implements tag index and speeds up binding lookup by tags.

I agree it's practical to see all changes in one place to understand how the high-level intentions map to underlying implementation details, and also to verify that all pieces fit together.

At the same time, this arrangement makes it very difficult to review the pull request in whole (because it's too large) and also to incrementally improve it by making small changes and reviewing only the recent changes. That's why we have a spike process, where we start with a throw-away prototype demonstrating feasibility of the chosen approach, and follow with a series of smaller pull requests that are easier to digest, review and incrementally improve.

I would really appreciate if you could split this PR into smaller chunks, it will make it much easier to review the proposed changes.

The first step could be the code & docs for new binding-level events and ContextEventListener API.

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch from 5fe3852 to 953f5ff Jan 14, 2020
@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch 3 times, most recently from 4e4f238 to c8068c6 Jan 14, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 14, 2020

It now depends on #4430

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch 2 times, most recently from c33a82c to 89ede5a Jan 14, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 15, 2020

@bajtos I landed #4430 and refactored this PR into 3 commits. PTAL.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 17, 2020

It now depends on #4430

Awesome!

refactored this PR into 3 commits

That's helpful too.

chore(context): tidy up context listeners for bind/unbind events

As far as I can see, this is not a chore, you are introducing a new feature ContextEventListener. Let's move this commit into a new PR please and make sure to include documentation.

See the thread in #4377 (comment):

ContextEventListener is a sync function as typical Node.js event listeners. The ContextObserver is async handler.

Sure, I can tell this part from the source code. I am asking for user-centric documentation - when should our users choose ContextObserver and when to choose ContextEventListener? As framework authors, we should set clear guidance here.

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch 2 times, most recently from e259f45 to e39cc65 Jan 17, 2020
@raymondfeng raymondfeng mentioned this pull request Jan 17, 2020
4 of 7 tasks complete
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 17, 2020

It now depends #4451

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch 5 times, most recently from 615dffa to d4589d5 Jan 17, 2020
Copy link
Contributor

agnes512 left a comment

I scan through the PR especially the tests. Most of them make sense to me. Just have one question.

@raymondfeng raymondfeng requested a review from bajtos Jan 23, 2020
packages/context/src/context.ts Outdated Show resolved Hide resolved
Copy link
Member

bajtos left a comment

鈽濓笍

raymondfeng added 2 commits Jan 6, 2020
This metadata allows optimization of context.find() to leverage tag index.
Fixes #4356

- create index for bindings by tag
- optimize find bindings by tag
- leverage findByTag for filterByTag to find matching bindings
@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch from d4589d5 to 3db144f Jan 23, 2020
@raymondfeng raymondfeng requested a review from bajtos Jan 23, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 23, 2020

@bajtos I also added a commit to extract context observer subscription logic into ContextSubscriptionManager class.

@raymondfeng raymondfeng force-pushed the improve-interceptor-perf branch from 64a0208 to a6778b3 Jan 23, 2020
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 27, 2020

@raymondfeng thank you for extracting smaller pieces out of the context class/file, the code structure looks much better now!

I am afraid I don't have enough energy to review the pull request in detail this week. I quickly skimmed through the changes and don't see any obvious major issues. Let's ask other owners of this functional area to review & approve the changes.

@deepakrkris @emonddr @jannyHou PTAL

Copy link
Contributor

jannyHou left a comment

馃憤

@raymondfeng raymondfeng merged commit 31ad9a5 into master Jan 27, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.006%) to 92.388%
Details
@raymondfeng raymondfeng deleted the improve-interceptor-perf branch Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can鈥檛 perform that action at this time.