[BEAM-13541] More intelligent caching of CoGBK values. #16354

robertwb · 2021-12-24T10:06:22Z

A minimal number of elements are cached for each tag, possibly in addition to a global number of elements cached.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
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.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

robertwb · 2021-12-28T07:46:00Z

Run Java PreCommit

aaltay · 2021-12-28T16:38:39Z

Run Java PreCommit

aaltay · 2021-12-28T16:39:04Z

FYI, reference to the previously failing precommit : https://ci-beam.apache.org/job/beam_PreCommit_Java_Phrase/4426/

robertwb · 2021-12-28T18:34:41Z

R: @emilymye or @aaltay or @lukecwik

aaltay

LGTM. Thank you.

aaltay · 2021-12-28T22:43:53Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

+  }
+
+  /**
+   * Assigns a monotonically increasing index to each item in teh underling Reiterator.


nit: s/teh/the

aaltay · 2021-12-28T23:00:27Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

-      this.unions = Iterators.peekingIterator(unions);
-      this.containsTag = containsTag;
+    // Used to keep track of what has been observed so far.
+    private final int[] lastObserved;


Why are these 3 variables are arrays? Only the index 0 is used.

They are zero-length arrays because we want to share these values among all copies of the reiterator. Basically they're like pointers. Added a comment to clarify.

emilymye

just noticed a few typos

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

emilymye · 2021-12-30T18:10:12Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/join/CoGbkResultTest.java

+  @Test
+  @SuppressWarnings("BoxedPrimitiveEquality")
+  public void testCachedResults() {
+    // Ensure we don't fail below due to odd VM settings.


out of curiousity, what odd VM settings?

java.lang.Integer.IntegerCache.high. Clarified to be more explicit.

lukecwik

It doesn't look like we will respect DEFAULT_IN_MEMORY_ELEMENT_COUNT. We will have at most DEFAULT_IN_MEMORY_ELEMENT_COUNT + (NUM_TAGS - 1) * DEFAULT_MIN_ELEMENTS_PER_TAG this is fine but wanted to confirm that this was your intent.

lukecwik · 2021-12-30T19:11:21Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -98,8 +104,7 @@ public CoGbkResult(
        throw new IllegalStateException(
            "union tag " + unionTag + " has no corresponding tuple tag in the result schema");
      }
-      List<Object> valueList = (List<Object>) valueMap.get(unionTag);
-      valueList.add(value.getValue());
+      valuesByTag.get(unionTag).add(value.getValue());
    }

    if (taggedIter.hasNext()) {


nit: use guard style

if (!taggedIter.hasNext()) { valueMap = (List) valuesByTag; } // If we get here, there were more elements than we can afford to // keep in memory, so we copy the re-iterable of remaining items // and append filtered views to each of the sorted lists computed earlier. ...

There are pros and cons to this, but I agree it's a slight improvement. Done.

lukecwik · 2021-12-30T19:21:18Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

+  private static class TagIterable<T> implements Iterable<T> {
+    int tag;
+    int cacheSize;
+    Supplier<Boolean> forceCache;


Leftover from a refactor, removing.

lukecwik · 2021-12-30T19:21:54Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -59,6 +60,8 @@

  private static final int DEFAULT_IN_MEMORY_ELEMENT_COUNT = 10_000;

+  private static final int DEFAULT_MIN_ELEMENTS_PER_TAG = 100;


Based upon the code it looks like this is used as a max per tag.

The idea is that we will cache always at least this many values per tag, regardless of whether DEFAULT_IN_MEMORY_ELEMENT_COUNT was "used up" for other tags. I'll clarify.

lukecwik · 2021-12-30T19:33:22Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/join/CoGbkResultTest.java

+  @Test
+  @SuppressWarnings("BoxedPrimitiveEquality")
+  public void testCachedResults() {
+    // Ensure we don't fail below due to a non-default java.lang.Integer.IntegerCache.high setting.


The assertion of which values are cached vs. re-created relies on not-so-small integers not being cached.

I understand that. I was trying to highlight the spacing issue in the comment. Also consider using a different type which isn't interned by the JVM.

Yeah. Strings have their own (more difficult to reason about) interning issues, and using a non-primitive gets really verbose here, which is why I stayed with integers.

lukecwik · 2021-12-30T23:11:14Z

Run Java PreCommit

robertwb · 2021-12-31T03:11:35Z

Running into BEAM-13575.

robertwb · 2021-12-31T03:11:42Z

Run Java PreCommit

robertwb · 2021-12-31T04:17:21Z

Same failure with unrelated org.apache.beam.runners.flink.FlinkRequiresStableInputTest.testParDoRequiresStableInput . I'm going to go ahead and merge so we can get a cherry pick going.

This is a follow-up to apache#16354

lukecwik · 2021-12-31T20:13:02Z

Spot bugs consistently fails with:

DLS 	Dead store to tail in new org.apache.beam.sdk.transforms.join.CoGbkResult(CoGbkResultSchema, Iterable, int, int)

Fixed in #16407

This is a follow-up to #16354

This is a follow-up to apache#16354

#16354, #16407) (#16421) Co-authored-by: Robert Bradshaw <robertwb@google.com> Co-authored-by: Lukasz Cwik <lukecwik@gmail.com>

…e#16354)" This reverts commit 3ab26ce.

robertwb added 4 commits December 23, 2021 18:45

[BEAM-13541] More intellegent caching of CoGBK values.

f6248ac

Get tests passing, spotless, cleanup.

447a1bc

Refactor to better resemble original, avoid overhead for small cogbks.

0fa2540

spotless

f603cc4

reuvenlax changed the title ~~[BEAM-13541] More intellegent caching of CoGBK values.~~ [BEAM-13541] More intelligent caching of CoGBK values. Dec 25, 2021

aaltay approved these changes Dec 28, 2021

View reviewed changes

typo, clarification

114f0e4

emilymye reviewed Dec 30, 2021

View reviewed changes

typos, clarification

b0fb758

lukecwik reviewed Dec 30, 2021

View reviewed changes

More cleanup, clarification.

982e8b6

lukecwik approved these changes Dec 30, 2021

View reviewed changes

lint

9526564

robertwb merged commit 3ab26ce into apache:master Dec 31, 2021

robertwb added a commit to robertwb/incubator-beam that referenced this pull request Dec 31, 2021

[BEAM-13541] More intelligent caching of CoGBK values. (apache#16354)

5f93fc3

lukecwik added a commit to lukecwik/incubator-beam that referenced this pull request Dec 31, 2021

[BEAM-13541] Fix spotbugs issue with DLS in tail

57537ee

This is a follow-up to apache#16354

lukecwik mentioned this pull request Dec 31, 2021

[BEAM-13541] Fix spotbugs issue with DLS in tail #16407

Merged

4 tasks

lukecwik added a commit that referenced this pull request Dec 31, 2021

[BEAM-13541] Fix spotbugs issue with DLS in tail (#16407)

bbcfa4f

This is a follow-up to #16354

emilymye pushed a commit to emilymye/beam that referenced this pull request Jan 4, 2022

[BEAM-13541] Fix spotbugs issue with DLS in tail (apache#16407)

51c59a9

This is a follow-up to apache#16354

emilymye mentioned this pull request Jan 4, 2022

[release-2.36.0][BEAM-13541] More intelligent caching of CoGBK values. (#16354, #16407) #16421

Merged

4 tasks

emilymye pushed a commit to emilymye/beam that referenced this pull request Jan 5, 2022

[BEAM-13541] More intelligent caching of CoGBK values. (apache#16354)

48e47a0

emilymye pushed a commit to emilymye/beam that referenced this pull request Jan 5, 2022

[BEAM-13541] Fix spotbugs issue with DLS in tail (apache#16407)

f252018

This is a follow-up to apache#16354

emilymye pushed a commit to emilymye/beam that referenced this pull request Jan 5, 2022

[BEAM-13541] More intelligent caching of CoGBK values. (apache#16354)

284f91e

emilymye pushed a commit to emilymye/beam that referenced this pull request Jan 5, 2022

[BEAM-13541] Fix spotbugs issue with DLS in tail (apache#16407)

01b9623

This is a follow-up to apache#16354

emilymye added a commit that referenced this pull request Jan 6, 2022

[release-2.36.0][BEAM-13541] More intelligent caching of CoGBK values. (

bd46e7b

#16354, #16407) (#16421) Co-authored-by: Robert Bradshaw <robertwb@google.com> Co-authored-by: Lukasz Cwik <lukecwik@gmail.com>

laraschmidt added a commit to laraschmidt/beam that referenced this pull request Mar 22, 2022

Rolling back apache#16354

8040379

tushar19 added a commit to twitter-forks/beam that referenced this pull request Mar 25, 2022

Revert "[BEAM-13541] More intelligent caching of CoGBK values. (apach…

19af122

…e#16354)" This reverts commit 3ab26ce.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-13541] More intelligent caching of CoGBK values. #16354

[BEAM-13541] More intelligent caching of CoGBK values. #16354

robertwb commented Dec 24, 2021

robertwb commented Dec 28, 2021

aaltay commented Dec 28, 2021

aaltay commented Dec 28, 2021

robertwb commented Dec 28, 2021

aaltay left a comment

aaltay Dec 28, 2021

robertwb Dec 29, 2021

aaltay Dec 28, 2021

robertwb Dec 29, 2021

emilymye left a comment

emilymye Dec 30, 2021

robertwb Dec 30, 2021

lukecwik left a comment

lukecwik Dec 30, 2021

robertwb Dec 30, 2021

lukecwik Dec 30, 2021

robertwb Dec 30, 2021

lukecwik Dec 30, 2021

robertwb Dec 30, 2021

lukecwik Dec 30, 2021

robertwb Dec 30, 2021

lukecwik Dec 30, 2021

robertwb Dec 30, 2021

lukecwik commented Dec 30, 2021

robertwb commented Dec 31, 2021

robertwb commented Dec 31, 2021

robertwb commented Dec 31, 2021

lukecwik commented Dec 31, 2021

		@@ -59,6 +60,8 @@

		private static final int DEFAULT_IN_MEMORY_ELEMENT_COUNT = 10_000;

		private static final int DEFAULT_MIN_ELEMENTS_PER_TAG = 100;

[BEAM-13541] More intelligent caching of CoGBK values. #16354

[BEAM-13541] More intelligent caching of CoGBK values. #16354

Conversation

robertwb commented Dec 24, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

robertwb commented Dec 28, 2021

aaltay commented Dec 28, 2021

aaltay commented Dec 28, 2021

robertwb commented Dec 28, 2021

aaltay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilymye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukecwik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukecwik commented Dec 30, 2021

robertwb commented Dec 31, 2021

robertwb commented Dec 31, 2021

robertwb commented Dec 31, 2021

lukecwik commented Dec 31, 2021

`ValidatesRunner` compliance status (on master branch)