lostluck commented on a change in pull request #15657:
URL: https://github.com/apache/beam/pull/15657#discussion_r731992100
##########
File path: sdks/go/pkg/beam/core/metrics/store.go
##########
@@ -174,36 +174,35 @@ type executionTracker struct {
numberOfTransitions int64
millisSinceLastTransition time.Duration
transitionsAtLastSample int64
-}
-
-// executionState is used to store as atomic.Value in Store.
-type pTransformState struct {
- pid string
- state bundleProcState
-}
-
-func newPTransformState(pid string, state bundleProcState) *pTransformState {
- return &pTransformState{pid: pid, state: state}
+ currentState bundleProcState
+ pid string
}
func SetPTransformState(ctx context.Context, state bundleProcState) {
if bctx, ok := ctx.(*beamCtx); ok {
- ps := newPTransformState(bctx.ptransformID, state)
- bctx.pStore.Store(ps)
- atomic.AddInt64(&bctx.transitions, 1)
+ pid := bctx.ptransformID
+ bctx.store.mu.Lock()
+ bctx.store.executionStore.pid = pid
+ bctx.store.executionStore.currentState = state
+ bctx.store.mu.Unlock()
+ atomic.AddInt64(&bctx.store.executionStore.numberOfTransitions,
1)
Review comment:
Interesting! That's a very good point. This is why we measure.
The reason atomic.Value is allocating so much is because it's wrapping
things into `interface{}`, which requires values to escape to the heap most of
the time. Since that's the case, we may want to use a 2nd lock for this sort of
management (rather than overburden the main store lock with multiple jobs).
This also means we can remove the atomic from transitons, and just use the lock
in question.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
