Repository navigation
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR resolves a performance issue in GlobalWindow where the singleton instance was repeatedly re-initialized via BoundedWindow.__init__ and _getTimestampFromProto() on every GlobalWindow() instantiation. Moving the initialization into __new__ (when _instance is None) and making __init__ a no-op properly preserves singleton semantics and provides a nice performance boost in window assignment.
🔍 General Feedback
- The implementation is clean, Idiomatic for Python
__new__-based singletons, and safely retains_endacross pickle/unpickle cycles.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
context: when benchmarking a batch FileIO write pipeline (https://github.com/Abacn/apache-beam-sstable), this path alone is found to cost 8.6% of total CPU time
|
`GlobalWindow` implements the singleton pattern via `__new__` by caching and returning `cls._instance`. However, because Python always invokes `__init__` on the instance returned by `__new__` (whenever `__new__` returns an instance of `cls`), defining `GlobalWindow.__init__` to call `super().__init__(GlobalWindow._getTimestampFromProto())` caused every `GlobalWindow()` call to re-parse the proto timestamp constant, allocate a new `Timestamp` object, and re-initialize `BoundedWindow._end`. Move the one-time `BoundedWindow.__init__` initialization into `GlobalWindow.__new__` when `cls._instance is None` and make `GlobalWindow.__init__` a no-op.
9d97e3e to
f06199b
Compare
|
Assigning reviewers: R: @damccorm for label python. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40910 +/- ##
==========================================
+ Coverage 79.83% 80.08% +0.24%
==========================================
Files 494 494
Lines 83575 83583 +8
==========================================
+ Hits 66722 66934 +212
+ Misses 16853 16649 -204
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|

GlobalWindowimplements the singleton pattern via__new__by caching and returningcls._instance. However, because Python always invokes__init__on the instance returned by__new__(whenever__new__returns an instance ofcls), definingGlobalWindow.__init__to callsuper().__init__(GlobalWindow._getTimestampFromProto())caused everyGlobalWindow()call to re-parse the proto timestamp constant, allocate a newTimestampobject, and re-initializeBoundedWindow._end.Move the one-time
BoundedWindow.__init__initialization intoGlobalWindow.__new__whencls._instance is Noneand makeGlobalWindow.__init__a no-op.==========
Benchmark Results (source: https://gist.github.com/Abacn/93fa3168450bf36838e70d853a77d2e3)
GlobalWindowInstantiate5.711e-077.120e-08GlobalWindowsAssign7.567e-072.399e-07WindowIntoWorker_GlobalWindows4.144e-062.934e-06WindowIntoWorker_FixedWindows5.970e-065.954e-06WriteImplBoundedPipeline1.643e-051.522e-05