You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/05/11 14:00:24 UTC
[GitHub] [beam] damccorm opened a new pull request, #17613: [BEAM-14347] Add some benchmarks for generic registration
damccorm opened a new pull request, #17613:
URL: https://github.com/apache/beam/pull/17613
This adds some basic benchmarks to prove that the generic registration works as well as the generated version. This isn't really perfect - it largely looks at pieces of the overall flow and doesn't provide a comprehensive full run of the DoFn, and it misses some cases (e.g. iters/emitters). This is because:
1) We can't have these benchmarks in the exec package because including the register package there induces a circular dependency and go test throws.
2) Much of the stuff we'd like to be able to easily test is private in exec. An example is that makeEmit is private, and its pretty challenging to test that specific functionality.
I didn't try _too_ hard to work around those constraints because I expect that once we've built up a decent history of load test runs that will actually be a better way to show that generic registration is just as good for more "real" use cases.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [x] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123855543
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @riteshghorse for label go.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123801567
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870454048
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
Review Comment:
```suggestion
type callerCustomTypeГint struct {
```
is this character intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870684630
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
+ fn func(CustomType) int
+}
+
+func funcMakerCustomTypeГint(fn interface{}) reflectx.Func {
+ f := fn.(func(CustomType) int)
+ return &callerCustomTypeГint{fn: f}
+}
+
+func (c *callerCustomTypeГint) Name() string {
+ return reflectx.FunctionName(c.fn)
+}
+
+func (c *callerCustomTypeГint) Type() reflect.Type {
+ return reflect.TypeOf(c.fn)
+}
+
+func (c *callerCustomTypeГint) Call(args []interface{}) []interface{} {
+ out0 := c.fn(args[0].(CustomType))
+ return []interface{}{out0}
+}
+
+func (c *callerCustomTypeГint) Call1x1(arg0 interface{}) interface{} {
+ return c.fn(arg0.(CustomType))
+}
+
+func wrapMakerFoo(fn interface{}) map[string]reflectx.Func {
+ dfn := fn.(*Foo)
+ return map[string]reflectx.Func{
+ "ProcessElement": reflectx.MakeFunc(func(a0 CustomType) int { return dfn.ProcessElement(a0) }),
+ }
+}
+
+func GeneratedOptimizationCalls() {
+ runtime.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ runtime.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ reflectx.RegisterFunc(reflect.TypeOf((*func(CustomType) int)(nil)).Elem(), funcMakerCustomTypeГint)
+ reflectx.RegisterStructWrapper(reflect.TypeOf((*Foo)(nil)).Elem(), wrapMakerFoo)
+}
+
+// BenchmarkMethodCalls measures the overhead of invoking several different methods after performing
+// different types of registration. The unoptimized calls don't perform any optimization. The
+// GenericRegistration calls first register the DoFn being used with this package's generic registration
+// functions. This is the preferred path for users. The GeneratedShims calls call various registration
+// functions, mirroring the behavior of the shims generated by the code generator. This is not the
+// recommended path for most users - if these are materially better than the generic benchmarks,
+// this package requires further optimization.
+//
+// BenchmarkMethodCalls/MakeFunc_Unoptimized-16 11480814 88.35 ns/op
+// BenchmarkMethodCalls/NewFn_Unoptimized-16 875199 1385 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_Unoptimized-16 1000000 1063 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GenericRegistration-16 16266259 72.07 ns/op
+// BenchmarkMethodCalls/NewFn_GenericRegistration-16 1000000 1108 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GenericRegistration-16 1000000 1052 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GeneratedShims#01-16 16400914 69.17 ns/op
+// BenchmarkMethodCalls/NewFn_GeneratedShims#01-16 1000000 1099 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GeneratedShims#01-16 1000000 1071 ns/op
+func BenchmarkMethodCalls(b *testing.B) {
Review Comment:
You're right - that was a silly oversight, I'd kinda lost track of some of my initial goals as I was writing these. As I was fixing it, I actually found an issue with the code we're generating (so thanks for calling that out!) that was causing it to not quite match the code generator, though it was still close.
I fixed both issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870688021
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
+ fn func(CustomType) int
+}
+
+func funcMakerCustomTypeГint(fn interface{}) reflectx.Func {
+ f := fn.(func(CustomType) int)
+ return &callerCustomTypeГint{fn: f}
+}
+
+func (c *callerCustomTypeГint) Name() string {
+ return reflectx.FunctionName(c.fn)
+}
+
+func (c *callerCustomTypeГint) Type() reflect.Type {
+ return reflect.TypeOf(c.fn)
+}
+
+func (c *callerCustomTypeГint) Call(args []interface{}) []interface{} {
+ out0 := c.fn(args[0].(CustomType))
+ return []interface{}{out0}
+}
+
+func (c *callerCustomTypeГint) Call1x1(arg0 interface{}) interface{} {
+ return c.fn(arg0.(CustomType))
+}
+
+func wrapMakerFoo(fn interface{}) map[string]reflectx.Func {
+ dfn := fn.(*Foo)
+ return map[string]reflectx.Func{
+ "ProcessElement": reflectx.MakeFunc(func(a0 CustomType) int { return dfn.ProcessElement(a0) }),
+ }
+}
+
+func GeneratedOptimizationCalls() {
+ runtime.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ runtime.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ reflectx.RegisterFunc(reflect.TypeOf((*func(CustomType) int)(nil)).Elem(), funcMakerCustomTypeГint)
+ reflectx.RegisterStructWrapper(reflect.TypeOf((*Foo)(nil)).Elem(), wrapMakerFoo)
+}
+
+// BenchmarkMethodCalls measures the overhead of invoking several different methods after performing
+// different types of registration. The unoptimized calls don't perform any optimization. The
+// GenericRegistration calls first register the DoFn being used with this package's generic registration
+// functions. This is the preferred path for users. The GeneratedShims calls call various registration
+// functions, mirroring the behavior of the shims generated by the code generator. This is not the
+// recommended path for most users - if these are materially better than the generic benchmarks,
+// this package requires further optimization.
+//
+// BenchmarkMethodCalls/MakeFunc_Unoptimized-16 11480814 88.35 ns/op
+// BenchmarkMethodCalls/NewFn_Unoptimized-16 875199 1385 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_Unoptimized-16 1000000 1063 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GenericRegistration-16 16266259 72.07 ns/op
+// BenchmarkMethodCalls/NewFn_GenericRegistration-16 1000000 1108 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GenericRegistration-16 1000000 1052 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GeneratedShims#01-16 16400914 69.17 ns/op
+// BenchmarkMethodCalls/NewFn_GeneratedShims#01-16 1000000 1099 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GeneratedShims#01-16 1000000 1071 ns/op
+func BenchmarkMethodCalls(b *testing.B) {
Review Comment:
And thus, revealing why I felt so strongly about this sort of thing :P. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123827838
# [Codecov](https://codecov.io/gh/apache/beam/pull/17613?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17613](https://codecov.io/gh/apache/beam/pull/17613?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (96a17c4) into [master](https://codecov.io/gh/apache/beam/commit/df67c817184acafc4adbc56a725eed4985053171?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (df67c81) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head 96a17c4 differs from pull request most recent head e207d69. Consider uploading reports for the commit e207d69 to get more accurate results
```diff
@@ Coverage Diff @@
## master #17613 +/- ##
=======================================
Coverage 73.95% 73.95%
=======================================
Files 693 693
Lines 91720 91720
=======================================
Hits 67832 67832
Misses 22639 22639
Partials 1249 1249
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `50.37% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17613?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17613?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [df67c81...e207d69](https://codecov.io/gh/apache/beam/pull/17613?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870556965
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
Review Comment:
Good question - surprisingly yes it is. This is intentionally mirroring [the registration functions generated by the code generator](https://github.com/apache/beam/blob/51067c123204a4951de3cba858edc96da354292b/sdks/go/pkg/beam/x/debug/debug.shims.go#L113), the naming helps point in that direction even more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123801572
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck merged pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
lostluck merged PR #17613:
URL: https://github.com/apache/beam/pull/17613
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870602939
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
Review Comment:
okay.. interesting!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] riteshghorse commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870454048
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
Review Comment:
```suggestion
type callerCustomTypeГint struct {
```
is this character `Г` intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123801573
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870594841
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
Review Comment:
Correct, as a rule, one can but *shouldn't* use such characters in one's Go identifiers.
So I picked symbols that were hard to type, to emphasize that one shouldn't be using any of these directly.
The generic approach avoids this entirely, since anything generated is already inaccessible to the user, since we don't expose those types to them at all. This allows us to changes things fairly freely, as long as the user interface doesn't change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17613:
URL: https://github.com/apache/beam/pull/17613#issuecomment-1123942222
R: @lostluck for final approval
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870693571
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
+ fn func(CustomType) int
+}
+
+func funcMakerCustomTypeГint(fn interface{}) reflectx.Func {
+ f := fn.(func(CustomType) int)
+ return &callerCustomTypeГint{fn: f}
+}
+
+func (c *callerCustomTypeГint) Name() string {
+ return reflectx.FunctionName(c.fn)
+}
+
+func (c *callerCustomTypeГint) Type() reflect.Type {
+ return reflect.TypeOf(c.fn)
+}
+
+func (c *callerCustomTypeГint) Call(args []interface{}) []interface{} {
+ out0 := c.fn(args[0].(CustomType))
+ return []interface{}{out0}
+}
+
+func (c *callerCustomTypeГint) Call1x1(arg0 interface{}) interface{} {
+ return c.fn(arg0.(CustomType))
+}
+
+func wrapMakerFoo(fn interface{}) map[string]reflectx.Func {
+ dfn := fn.(*Foo)
+ return map[string]reflectx.Func{
+ "ProcessElement": reflectx.MakeFunc(func(a0 CustomType) int { return dfn.ProcessElement(a0) }),
+ }
+}
+
+func GeneratedOptimizationCalls() {
+ runtime.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ runtime.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ reflectx.RegisterFunc(reflect.TypeOf((*func(CustomType) int)(nil)).Elem(), funcMakerCustomTypeГint)
+ reflectx.RegisterStructWrapper(reflect.TypeOf((*Foo)(nil)).Elem(), wrapMakerFoo)
+}
+
+// BenchmarkMethodCalls measures the overhead of invoking several different methods after performing
+// different types of registration. The unoptimized calls don't perform any optimization. The
+// GenericRegistration calls first register the DoFn being used with this package's generic registration
+// functions. This is the preferred path for users. The GeneratedShims calls call various registration
+// functions, mirroring the behavior of the shims generated by the code generator. This is not the
+// recommended path for most users - if these are materially better than the generic benchmarks,
+// this package requires further optimization.
+//
+// BenchmarkMethodCalls/MakeFunc_Unoptimized-16 11480814 88.35 ns/op
+// BenchmarkMethodCalls/NewFn_Unoptimized-16 875199 1385 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_Unoptimized-16 1000000 1063 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GenericRegistration-16 16266259 72.07 ns/op
+// BenchmarkMethodCalls/NewFn_GenericRegistration-16 1000000 1108 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GenericRegistration-16 1000000 1052 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GeneratedShims#01-16 16400914 69.17 ns/op
+// BenchmarkMethodCalls/NewFn_GeneratedShims#01-16 1000000 1099 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GeneratedShims#01-16 1000000 1071 ns/op
+func BenchmarkMethodCalls(b *testing.B) {
Review Comment:
Yep - good instincts!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870605529
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
+ fn func(CustomType) int
+}
+
+func funcMakerCustomTypeГint(fn interface{}) reflectx.Func {
+ f := fn.(func(CustomType) int)
+ return &callerCustomTypeГint{fn: f}
+}
+
+func (c *callerCustomTypeГint) Name() string {
+ return reflectx.FunctionName(c.fn)
+}
+
+func (c *callerCustomTypeГint) Type() reflect.Type {
+ return reflect.TypeOf(c.fn)
+}
+
+func (c *callerCustomTypeГint) Call(args []interface{}) []interface{} {
+ out0 := c.fn(args[0].(CustomType))
+ return []interface{}{out0}
+}
+
+func (c *callerCustomTypeГint) Call1x1(arg0 interface{}) interface{} {
+ return c.fn(arg0.(CustomType))
+}
+
+func wrapMakerFoo(fn interface{}) map[string]reflectx.Func {
+ dfn := fn.(*Foo)
+ return map[string]reflectx.Func{
+ "ProcessElement": reflectx.MakeFunc(func(a0 CustomType) int { return dfn.ProcessElement(a0) }),
+ }
+}
+
+func GeneratedOptimizationCalls() {
+ runtime.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ runtime.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ reflectx.RegisterFunc(reflect.TypeOf((*func(CustomType) int)(nil)).Elem(), funcMakerCustomTypeГint)
+ reflectx.RegisterStructWrapper(reflect.TypeOf((*Foo)(nil)).Elem(), wrapMakerFoo)
+}
+
+// BenchmarkMethodCalls measures the overhead of invoking several different methods after performing
+// different types of registration. The unoptimized calls don't perform any optimization. The
+// GenericRegistration calls first register the DoFn being used with this package's generic registration
+// functions. This is the preferred path for users. The GeneratedShims calls call various registration
+// functions, mirroring the behavior of the shims generated by the code generator. This is not the
+// recommended path for most users - if these are materially better than the generic benchmarks,
+// this package requires further optimization.
+//
+// BenchmarkMethodCalls/MakeFunc_Unoptimized-16 11480814 88.35 ns/op
+// BenchmarkMethodCalls/NewFn_Unoptimized-16 875199 1385 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_Unoptimized-16 1000000 1063 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GenericRegistration-16 16266259 72.07 ns/op
+// BenchmarkMethodCalls/NewFn_GenericRegistration-16 1000000 1108 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GenericRegistration-16 1000000 1052 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GeneratedShims#01-16 16400914 69.17 ns/op
+// BenchmarkMethodCalls/NewFn_GeneratedShims#01-16 1000000 1099 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GeneratedShims#01-16 1000000 1071 ns/op
+func BenchmarkMethodCalls(b *testing.B) {
Review Comment:
So none of these actually test "calling" the DoFns and similar. This is just measuring all the existing pipeline construction time + bundle unmarshal time scaffolding, that would not be meaningfully affected by any of the generated code or not.
It's nice to validate that things are lightly faster this way or at least comparable, but it doesn't demonstrate the value of the wrappings, or that the generic approach is equivalent to the code generated approach.
To test the generated code, one would need to be calling `aFunc.Call([]interface{CustomType{ A: 4 }})` or similar. And likely moving the parameter definition out of the loop so it's not bogged down by unrelated slice allocation overhead, or also having a `a1x1Func` (of type `reflectx.Func1x1`) so `Call1x1(CustomType{ A: 4 })` could be used.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #17613: [BEAM-14347] Add some benchmarks for generic registration
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17613:
URL: https://github.com/apache/beam/pull/17613#discussion_r870684926
##########
sdks/go/pkg/beam/register/register_test.go:
##########
@@ -672,3 +675,133 @@ func (fn *PartialCombiner2) AddInput(i int, c CustomType) int {
func (fn *PartialCombiner2) MergeAccumulators(i1 int, i2 int) int {
return i1 + i2
}
+
+// Foo is a struct with a method for measuring method invocation
+// overhead for StructuralDoFns.
+type Foo struct {
+ A int
+}
+
+// ProcessElement is a method for measuring a baseline of structural dofn overhead.
+func (f *Foo) ProcessElement(b CustomType) int {
+ return f.A + b.val
+}
+
+func MakeMultiEdge(f *graph.DoFn) graph.MultiEdge {
+ return graph.MultiEdge{
+ DoFn: f,
+ }
+}
+
+type callerCustomTypeГint struct {
+ fn func(CustomType) int
+}
+
+func funcMakerCustomTypeГint(fn interface{}) reflectx.Func {
+ f := fn.(func(CustomType) int)
+ return &callerCustomTypeГint{fn: f}
+}
+
+func (c *callerCustomTypeГint) Name() string {
+ return reflectx.FunctionName(c.fn)
+}
+
+func (c *callerCustomTypeГint) Type() reflect.Type {
+ return reflect.TypeOf(c.fn)
+}
+
+func (c *callerCustomTypeГint) Call(args []interface{}) []interface{} {
+ out0 := c.fn(args[0].(CustomType))
+ return []interface{}{out0}
+}
+
+func (c *callerCustomTypeГint) Call1x1(arg0 interface{}) interface{} {
+ return c.fn(arg0.(CustomType))
+}
+
+func wrapMakerFoo(fn interface{}) map[string]reflectx.Func {
+ dfn := fn.(*Foo)
+ return map[string]reflectx.Func{
+ "ProcessElement": reflectx.MakeFunc(func(a0 CustomType) int { return dfn.ProcessElement(a0) }),
+ }
+}
+
+func GeneratedOptimizationCalls() {
+ runtime.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*Foo)(nil)).Elem())
+ runtime.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ schema.RegisterType(reflect.TypeOf((*CustomType)(nil)).Elem())
+ reflectx.RegisterFunc(reflect.TypeOf((*func(CustomType) int)(nil)).Elem(), funcMakerCustomTypeГint)
+ reflectx.RegisterStructWrapper(reflect.TypeOf((*Foo)(nil)).Elem(), wrapMakerFoo)
+}
+
+// BenchmarkMethodCalls measures the overhead of invoking several different methods after performing
+// different types of registration. The unoptimized calls don't perform any optimization. The
+// GenericRegistration calls first register the DoFn being used with this package's generic registration
+// functions. This is the preferred path for users. The GeneratedShims calls call various registration
+// functions, mirroring the behavior of the shims generated by the code generator. This is not the
+// recommended path for most users - if these are materially better than the generic benchmarks,
+// this package requires further optimization.
+//
+// BenchmarkMethodCalls/MakeFunc_Unoptimized-16 11480814 88.35 ns/op
+// BenchmarkMethodCalls/NewFn_Unoptimized-16 875199 1385 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_Unoptimized-16 1000000 1063 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GenericRegistration-16 16266259 72.07 ns/op
+// BenchmarkMethodCalls/NewFn_GenericRegistration-16 1000000 1108 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GenericRegistration-16 1000000 1052 ns/op
+//
+// BenchmarkMethodCalls/MakeFunc_GeneratedShims#01-16 16400914 69.17 ns/op
+// BenchmarkMethodCalls/NewFn_GeneratedShims#01-16 1000000 1099 ns/op
+// BenchmarkMethodCalls/EncodeMultiEdge_GeneratedShims#01-16 1000000 1071 ns/op
+func BenchmarkMethodCalls(b *testing.B) {
Review Comment:
FWIW, it was specifically with the 1x1 case, so I think having both a call and a call1x1 benchmark is valuable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org