[
https://issues.apache.org/jira/browse/BEAM-4468?focusedWorklogId=202928&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-202928
]
ASF GitHub Bot logged work on BEAM-4468:
----------------------------------------
Author: ASF GitHub Bot
Created on: 22/Feb/19 22:45
Start Date: 22/Feb/19 22:45
Worklog Time Spent: 10m
Work Description: lostluck commented on pull request #7927: [BEAM-4468]
Go SDK Combiner Lifting cache cap & eviction.
URL: https://github.com/apache/beam/pull/7927#discussion_r259535507
##########
File path: sdks/go/pkg/beam/core/runtime/exec/combine.go
##########
@@ -318,6 +318,27 @@ func (n *LiftedCombine) ProcessElement(ctx
context.Context, value *FullValue, va
return n.fail(err)
}
+ // TODO(BEAM-4468): replace with some better implementation
+ // once adding dependencies is easier.
+ // Arbitrary limit until a broader improvement can be demonstrated.
+ const cacheMax = 2000
+ if len(n.cache) > cacheMax {
Review comment:
Excellent question!
The initial implementation did that approach but it ended up providing
insufficient combining for extremely large bundles, causing the pessimal case
of everything getting sent to the GBK un-combined. eg. If there are 2001
distinct keys, and they cycle in any order, except the 2001th key is also the
4002nd key , that strategy will lead to no combining at all, since we don't
provide any opportunity for the overlapping sets to meet.
The observation then is that we need to provide the opportunity for
combining, which means keeping the cache as populated as possible. The bounding
is to control memory growth, in particular, for Very Large Bundles.
You're right that the random eviction strategy will sometimes have sequences
where keys are being evicted and possibly even a key that's about to arrive
will be evicted. However, without some extra buffering between DoFns, that's
impossible to guess without far more sophisticated code.
Random is just a good "placebo" algorithm for picking evictees since
anything else is harder get right.
In particular, it's safe in Go to delete map entries in a loop, and the
iteration order is random.
I'm pretty sure there's a paper somewhere about the efficacy of eviction
different eviction patterns on ~random~ datasets, but I'm not academically
inclined enough to find it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 202928)
Time Spent: 2h (was: 1h 50m)
> Go SDK-Tune in memory pre-combine caching for Lifted Combines.
> --------------------------------------------------------------
>
> Key: BEAM-4468
> URL: https://issues.apache.org/jira/browse/BEAM-4468
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
> Reporter: Robert Burke
> Assignee: Robert Burke
> Priority: Minor
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Requires [BEAM-4276|https://issues.apache.org/jira/browse/BEAM-4276] to be
> completed first.
> Additional performance tweaks to the in memory per-key accumulator cache
> during the pre-combine phase of a lifted combine.
> This can include any of
> * capping the number of key-accumulator in the cache, and draining them
> eagerly after x elements seen, or evicting keys by some heuristic after the
> cache has seen X distinct keys.
> * providing a counter on cache size (key/element counts) exposable through
> the metrics interface or another runner standard counter, to permit observing
> the cache's status, especially if it could grow without bound within a bundle.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
