Big Performance Improvement #29
Conversation
|
This is great, I didn't know that we had this kind of performance hit by getting the class' name that way. The way you propose doing this, however, exposes a risk: if the name contains a typo then the bypass API will fail if used with the correct handler name. I'm sure this is a small price to pay, though. |
|
I’m researching internally why this is taking so long. I think a safer option is to memoize the value.
…On Apr 21, 2020, 17:11 -0400, Renato Oliveira ***@***.***>, wrote:
This is great, I didn't know that we had this kind of performance hit by getting the class' name that way.
The way you propose doing this, however, exposes a risk: if the name contains a typo then the bypass API will fail if used with the correct handler name. I'm sure this is a small price to pay, though.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Have you found out anything more, @codefriar? Can this be merged, or do you have another solution? Thanks! |
|
I don't really like losing the automated handler name resolution and forcing the name through a parent constructor. A really simple solution would just be to cache the It's not expensive to do it once, but it's being called repeatedly in the current solution which is what is causing the CPU cycles. |
|
I have another PR I'm holding until PR30 is approved; if @timbarsotti doesn't get to it first, I'll add this as well. |
|
@kevinohara80 the issue I see with a static variable is when multiple handlers are in play in a single execution context. By adding this change, it doesn't remove the dynamic reading of the TriggerHandler, but allows the dynamic to be overridden with a super constructor. TriggerHandler works fine without calling super(). @renatoliveira yes - risk is there. |
|
@timbarsotti I'm not following this...
So you're saying you'd want to be able to compute some sort of dynamic trigger handler name at runtime? P.S. I see that the handlerName is being memoized which is great. I also don't see an issue with overloading the constructor with a supplied handlerName. Maybe what threw me off initially was the change to the docs mentioning performance improvements by using the overloaded constructor. The BIG performance improvement is the memoizing of the handler name. Supplying a handlerName in the constructor is not going to be a noticeable difference in performance as the alternative is only going to compute a handlerName once. |
|
Any reason this PR is not yet accepted? |
| if(String.isBlank(this.handlerName)) { | ||
| this.handlerName = String.valueOf(this).substring(0,String.valueOf(this).indexOf(':')); | ||
| } | ||
| return handlerName; |
There was a problem hiding this comment.
| return handlerName; | |
| return this.handlerName; |
…er handler name and method to set the name (see kevinohara80#29) to improve performance. Added new constructor to accept trigger handler name. Added test method for new constructor. Updated README.md to show examples of using new constructor.
|
@kevinohara80 , just reviving the discussion :), and explore if this one or what @vr8hub had around this? |
Noticed this consumes a lot of CPU time with lots of triggers. Made a significant improvement to TriggerHandler to improve this. Write found here: http://timbarsotti.com/2020/04/21/triggerhandler-performance/