WriteOutBytes improvements by adarshsanjeev · Pull Request #16698 · apache/druid

adarshsanjeev · 2024-07-08T06:00:26Z

This PR generally improves the working of WriteOutBytes and WriteOutMedium. Some analysis of usage of TmpFileSegmentWriteOutMedium shows that they periodically get used for very small things. The overhead of creating a tmp file is actually very large. To improve the performance in these cases, this PR modifies TmpFileSegmentWriteOutMedium to return a heap-based WriteOutBytes that falls back to making a tmp file when it actually fills up.

Increases the memory in FileWriteOutBytes and switches to using direct memory to improve performance.
Refactor TmpFileSegmentWriteOutMedium to use a heap based WriteOutBytes before falling back to a temporary file.
Modify FileWriteOutBytes's behavior on writing a value larger than its buffer. Previously, it used to throw an exception. This PR modifies it to write the value to the file directly instead, bypassing the buffer.
Adds some additional logging around the usage of write out bytes.

This PR has:

clintropolis · 2024-07-08T06:54:51Z

-        file.toPath(),
-        StandardOpenOption.READ,
-        StandardOpenOption.WRITE
+    return new LazilyAllocatingHeapWriteOutBytes(


why not implement a new SegmentWriteOutMedium instead of changing the behavior of an existing one?

Because it has broad-reaching performance improving impacts? The alternative would be to create a new thing, update everything to use it and then delete the old, which is basically the same as changing in place. The reason to leave the old one would be if we wanted to maintain configurability to do the old thing, but I don't know why that would be useful here.

yea im for the strategy in general, just kind of worried about the slightly larger memory footprint here without any form of escape hatch. This looks like it can grow up to 16k (up from the 4k heap buffer of the old FileWriteOutBytes buffer that has been replaced with a direct buffer in this PR). Its probably cool, I'm just being nervous.

I was suggesting mostly just making a new thing and making it the default and leave the old thing just in case, but i can be convinced that this is close enough to not matter i think.

Actually it is 32KB off heap instead of 4KB heap. I wonder if that could be a problem with large number of columns. Although with large number of columns we also see heap getting exhausted as well. So this might even be better in some cases?

imply-cheddar

LGTM

imply-cheddar · 2024-07-09T06:04:32Z

-        file.toPath(),
-        StandardOpenOption.READ,
-        StandardOpenOption.WRITE
+    return new LazilyAllocatingHeapWriteOutBytes(


Because it has broad-reaching performance improving impacts? The alternative would be to create a new thing, update everything to use it and then delete the old, which is basically the same as changing in place. The reason to leave the old one would be if we wanted to maintain configurability to do the old thing, but I don't know why that would be useful here.

abhishekagarwal87 · 2024-07-09T06:16:01Z

Can you add please tests to cover the code changes? E.g. is there a test already that makes sure that filesCreated is changed correctly in TmpFileSegmentWriteOutMedium class.

rohangarg

Mostly LGTM!
Left a minor comment for understanding.
I would want to wait for the memory allocation thread to be concluded before merging.

rohangarg · 2024-07-26T12:14:53Z

+            catch (Exception e) {
+              throw new RuntimeException(e);
+            }
+            Assert.assertEquals(1, writeOutMedium.getNumLocallyCreated());


should this count be 3 instead of 1? I think I'm misunderstanding the test.

That's the count on the thread, which is one. The total count asserted outside is 3.

rohangarg · 2024-07-29T13:17:05Z

-        file.toPath(),
-        StandardOpenOption.READ,
-        StandardOpenOption.WRITE
+    return new LazilyAllocatingHeapWriteOutBytes(


Actually it is 32KB off heap instead of 4KB heap. I wonder if that could be a problem with large number of columns. Although with large number of columns we also see heap getting exhausted as well. So this might even be better in some cases?

imply-cheddar · 2024-08-06T19:28:19Z

@adarshsanjeev I submitted a PR (adarshsanjeev#1) to your branch that adds the old TmpFileSegmentWriteOutMedium back as a legacy option. Please merge that PR, which will cause this to update. It should address the concerns that the current changes to the code don't have a method of rolling back to the old behavior if something does go sideways.

Have it as a legacy option

Reintroduce old TmpFileSegmentWriteOutMedium

…rovement

clintropolis · 2024-08-19T23:18:48Z

 @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = TmpFileSegmentWriteOutMediumFactory.class)
 @JsonSubTypes(value = {
    @JsonSubTypes.Type(name = "tmpFile", value = TmpFileSegmentWriteOutMediumFactory.class),
+    @JsonSubTypes.Type(name = "legacyTmpFile", value = LegacyTmpFileSegmentWriteOutMediumFactory.class),


nit: i think putting legacy in the names of config stuff is not usually great to do since other things could become legacy too i suppose, but i don't feel super strongly either way i guess. We should maybe update docs to indicate that this exists? https://druid.apache.org/docs/latest/configuration/#segmentwriteoutmediumfactory

WriteOutBytes improvements

aa5ac2f

github-actions Bot added the Area - Segment Format and Ser/De label Jul 8, 2024

clintropolis reviewed Jul 8, 2024

View reviewed changes

imply-cheddar approved these changes Jul 9, 2024

View reviewed changes

adarshsanjeev mentioned this pull request Jul 9, 2024

Add a segmentMorphFactory to MSQ Datasource Destination #16692

Closed

10 tasks

adarshsanjeev added 7 commits July 13, 2024 22:59

Add tests

6479a5e

Handle closing better

b8bae76

Fix tests

8b1bc68

Add tests

a938127

Fix checkstyle

4bf8071

Add tests

003387c

Fix tests

71d2a25

rohangarg reviewed Jul 29, 2024

View reviewed changes

imply-cheddar and others added 4 commits August 6, 2024 14:30

Reintroduce old TmpFileSegmentWriteOutMedium

ef20cbc

Have it as a legacy option

Merge pull request #1 from imply-cheddar/keep-legacy-write-out-bytes

c80aede

Reintroduce old TmpFileSegmentWriteOutMedium

Fix checkstyle

d12b9b7

Merge remote-tracking branch 'origin/master' into write-out-bytes-imp…

f1b9bc3

…rovement

clintropolis approved these changes Aug 19, 2024

View reviewed changes

adarshsanjeev merged commit e2516d9 into apache:master Aug 23, 2024

kfaraz added this to the 31.0.0 milestone Oct 4, 2024

kfaraz mentioned this pull request Oct 11, 2024

[DRAFT] 31.0.0 Release Notes #17332

Closed

Conversation

adarshsanjeev commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imply-cheddar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 commented Jul 9, 2024

Uh oh!

rohangarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imply-cheddar commented Aug 6, 2024

Uh oh!

clintropolis Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adarshsanjeev commented Jul 8, 2024 •

edited

Loading

clintropolis Aug 19, 2024 •

edited

Loading