Microbenchmark for Trigger Filter

[slinkydeveloper]

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

This adds microbenchmarks to test implementations of eventfilter.Filter.

Note: although it doesn't make sense to test for the attributes filter the creation of the filter instance, it makes sense for more complex filters like in the context of #3359 and #4495, where creating the filter means transforming the user input in some middle representation (like an AST or a schema instance) which is not trivial to create

Proposed Changes

• Add generic microbenchmark to test creation and run of the filter implementation
• Add attributes microbenchmark tests

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [slinkydeveloper]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Merging #4517 (253486d) into master (3b560a3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4517   +/-   ##
=======================================
  Coverage   81.27%   81.27%           
=======================================
  Files         284      284           
  Lines        8017     8017           
=======================================
  Hits         6516     6516           
  Misses       1113     1113           
  Partials      388      388           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b560a3...4a0db25. Read the comment docs.

[zhongduo]

/assign

[zhongduo]

/lgtm
/hold in case you want to add comment to the RunFilterBenchmarks, which is shared by future usage too I guess. Unhold it if you decide not to do it.

[slinkydeveloper]

@zhongduo comment added, to explain what benchmarks we're running

/unhold

[slinkydeveloper]

@zhongduo Can you re-lgtm please? Adding one comment require lgtm again 😞

[zhongduo]

/lgtm Thanks.

[pierDipi]

/lgtm

[pierDipi]

I think we should rename those types, the usage it's unreadable, too many filter.

[pierDipi]

@slinkydeveloper WDYT?

[slinkydeveloper]

Which names do you propose?

[zhongduo]

IIUC, these are just to avoid DCE. So the variables are really not designed to be used. I would suggest to use lower case if I want to be picky. Also sth like filterForNoDCE.

[slinkydeveloper]

To be 100% you exclude DCE, you need variables to be exported

[pierDipi]

I meant package.type not variable names.

trigger.Filter
trigger.FilterResult

wdyt?

[pierDipi]

or filter.Interface and filter.Result

[slinkydeveloper]

And where should this trigger module should live? Under eventing/pkg/trigger? Not sure, to me eventfilter, although it's repeated a couple of times, sounds like a good name (and note, this code is not specific at all to trigger, in fact at some point it was even discussed to implement filters at source level)

[zhongduo]

hmm, is it possible to do a quick test? I have a compiler background, though for C/C++. But as long as a variable can be used outside the function, DCE will have trouble optimizing it. So in this case you can have an in-package function that monitor these variables, which will be effectively the same as exported variables since they can be used outside the function.

…

On Thu, Nov 12, 2020 at 12:16 PM Francesco Guardiani < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/eventfilter/benchmarks/common_benchmark_test.go <#4517 (comment)>: > +var Filter eventfilter.Filter +var Result eventfilter.FilterResult To be 100% you exclude DCE, you need variables to be exported — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4517 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACE6CNFEBBYDGMFHMOK4VMDSPQJ7JANCNFSM4TTMLMMA> .

[slinkydeveloper]

hmm, is it possible to do a quick test?

WDYM? Are you talking about having the variable exported or not?

[slinkydeveloper]

Even if your compiler cannot optimize today an unexported variable, it might still do it in future (due to the evolution of the escape analysis), that's why it generally makes sense to make it always exported