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

raymondfeng · 2020-01-07T16:13:51Z

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:

Make Binding to be EventEmitter - emitting events when binding scope/tags/value are changed
Set up listeners in Context to react to binding events to maintain an index of bindings by tag
Optimize Context.findByTag to leverage binding index if possible
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 👈

bajtos

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.

bajtos · 2020-01-09T13:43:44Z

packages/context/src/binding.ts

@@ -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 {


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?

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.

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

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.

bajtos · 2020-01-09T13:46:34Z

packages/context/src/context.ts

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


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.

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

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

bajtos · 2020-01-09T13:55:48Z

packages/context/src/context.ts

+    ) {
+      return this.find(filterByTag(tagFilter));
+    }
+    return this._findByTagIndex(tagFilter);


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

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

raymondfeng · 2020-01-10T17:20:24Z

@bajtos PTAL.

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts

bajtos · 2020-01-14T15:42:48Z

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 · 2020-01-14T21:34:57Z

It now depends on #4430

raymondfeng · 2020-01-15T21:18:21Z

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

bajtos · 2020-01-17T09:45:04Z

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.

packages/context/src/binding-filter.ts

raymondfeng · 2020-01-17T16:27:19Z

It now depends #4451

agnes512

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

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts

packages/context/src/context.ts

bajtos

☝️

packages/context/src/context.ts

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 · 2020-01-23T17:43:51Z

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

bajtos · 2020-01-27T09:55:57Z

@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

jannyHou

👍

raymondfeng requested a review from bajtos as a code owner January 7, 2020 16:13

raymondfeng mentioned this pull request Jan 7, 2020

Performance degradation #4356

Closed

dhmlau added the Performance Issues related to runtime performance label Jan 8, 2020

dhmlau assigned raymondfeng Jan 8, 2020

bajtos suggested changes Jan 9, 2020

View reviewed changes

raymondfeng force-pushed the improve-interceptor-perf branch 5 times, most recently from d040e6b to 213642c Compare January 10, 2020 17:16

raymondfeng requested a review from bajtos January 10, 2020 17:19

raymondfeng force-pushed the improve-interceptor-perf branch from 213642c to ab3fe0f Compare January 10, 2020 19:13

raymondfeng changed the title ~~feat: improve interceptor perf~~ feat: improve context.find() and interceptor perf Jan 10, 2020

raymondfeng changed the title ~~feat: improve context.find() and interceptor perf~~ feat: improve context.find/findByTag() and interceptor perf Jan 10, 2020

bajtos reviewed Jan 13, 2020

View reviewed changes

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts Outdated Show resolved Hide resolved

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts Outdated Show resolved Hide resolved

raymondfeng force-pushed the improve-interceptor-perf branch from ab3fe0f to 5fe3852 Compare January 13, 2020 16:30

raymondfeng requested a review from bajtos January 13, 2020 17:27

raymondfeng force-pushed the improve-interceptor-perf branch from 5fe3852 to 953f5ff Compare January 14, 2020 17:27

raymondfeng mentioned this pull request Jan 14, 2020

feat(context): make bindings as event emitters to report changes #4430

Merged

7 tasks

raymondfeng force-pushed the improve-interceptor-perf branch 3 times, most recently from 4e4f238 to c8068c6 Compare January 14, 2020 21:33

raymondfeng force-pushed the improve-interceptor-perf branch 2 times, most recently from c33a82c to 89ede5a Compare January 15, 2020 21:17

emonddr approved these changes Jan 16, 2020

View reviewed changes

bajtos reviewed Jan 17, 2020

View reviewed changes

packages/context/src/binding-filter.ts Show resolved Hide resolved

packages/context/src/binding-filter.ts Show resolved Hide resolved

raymondfeng force-pushed the improve-interceptor-perf branch 2 times, most recently from e259f45 to e39cc65 Compare January 17, 2020 16:23

raymondfeng mentioned this pull request Jan 17, 2020

feat(context): improve context listener and events #4451

Merged

7 tasks

raymondfeng force-pushed the improve-interceptor-perf branch 5 times, most recently from 615dffa to d4589d5 Compare January 21, 2020 18:17

agnes512 reviewed Jan 21, 2020

View reviewed changes

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts Show resolved Hide resolved

raymondfeng requested a review from bajtos January 23, 2020 15:24

bajtos reviewed Jan 23, 2020

View reviewed changes

packages/context/src/context.ts Outdated Show resolved Hide resolved

bajtos reviewed Jan 23, 2020

View reviewed changes

packages/context/src/context.ts Show resolved Hide resolved

raymondfeng added 2 commits January 23, 2020 08:02

feat(context): keep binding tag pattern for BindingTagFilter

6e1af29

This metadata allows optimization of context.find() to leverage tag index.

feat(context): index bindings by tag to speed up matching by tag

3db144f

Fixes #4356 - create index for bindings by tag - optimize find bindings by tag - leverage findByTag for filterByTag to find matching bindings

raymondfeng force-pushed the improve-interceptor-perf branch from d4589d5 to 3db144f Compare January 23, 2020 16:43

raymondfeng requested a review from bajtos January 23, 2020 17:42

feat(context): refactor context observer subscription into a new class

a6778b3

raymondfeng force-pushed the improve-interceptor-perf branch from 64a0208 to a6778b3 Compare January 23, 2020 17:55

jannyHou approved these changes Jan 27, 2020

View reviewed changes

emonddr approved these changes Jan 27, 2020

View reviewed changes

raymondfeng merged commit 31ad9a5 into master Jan 27, 2020

raymondfeng deleted the improve-interceptor-perf branch January 27, 2020 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raymondfeng commented Jan 7, 2020 •

edited

Loading

bajtos left a comment

bajtos Jan 9, 2020 •

edited

Loading

raymondfeng Jan 9, 2020

raymondfeng Jan 10, 2020

bajtos Jan 13, 2020

bajtos Jan 9, 2020

raymondfeng Jan 9, 2020

bajtos Jan 13, 2020

bajtos Jan 9, 2020

raymondfeng Jan 9, 2020 •

edited

Loading

raymondfeng commented Jan 10, 2020

bajtos commented Jan 14, 2020

raymondfeng commented Jan 14, 2020

raymondfeng commented Jan 15, 2020

bajtos commented Jan 17, 2020

raymondfeng commented Jan 17, 2020

agnes512 left a comment

bajtos left a comment •

edited

Loading

raymondfeng commented Jan 23, 2020

bajtos commented Jan 27, 2020

jannyHou left a comment

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

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

Conversation

raymondfeng commented Jan 7, 2020 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

bajtos Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

raymondfeng Jan 9, 2020

Choose a reason for hiding this comment

raymondfeng Jan 10, 2020

Choose a reason for hiding this comment

bajtos Jan 13, 2020

Choose a reason for hiding this comment

bajtos Jan 9, 2020

Choose a reason for hiding this comment

raymondfeng Jan 9, 2020

Choose a reason for hiding this comment

bajtos Jan 13, 2020

Choose a reason for hiding this comment

bajtos Jan 9, 2020

Choose a reason for hiding this comment

raymondfeng Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

raymondfeng commented Jan 10, 2020

bajtos commented Jan 14, 2020

raymondfeng commented Jan 14, 2020

raymondfeng commented Jan 15, 2020

bajtos commented Jan 17, 2020

raymondfeng commented Jan 17, 2020

agnes512 left a comment

Choose a reason for hiding this comment

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

raymondfeng commented Jan 23, 2020

bajtos commented Jan 27, 2020

jannyHou left a comment

Choose a reason for hiding this comment

raymondfeng commented Jan 7, 2020 •

edited

Loading

bajtos Jan 9, 2020 •

edited

Loading

raymondfeng Jan 9, 2020 •

edited

Loading

bajtos left a comment •

edited

Loading