Add tracking of memory allocation per operator by mbasmanova · Pull Request #14191 · prestodb/presto

mbasmanova · 2020-03-02T21:45:26Z

Here is a snippet of the query JSON fetched from the coordinator featuring new fields: addInput/getOutput/finishAllocation

  }, {
      "stageId" : 1,
      "stageExecutionId" : 0,
      "pipelineId" : 1,
      "operatorId" : 1,
      "planNodeId" : "4",
      "operatorType" : "HashAggregationOperator",
      "totalDrivers" : 408,
      "addInputCalls" : 1,
      "addInputWall" : "23.92ms",
      "addInputCpu" : "22.66ms",
      "addInputAllocation" : "3.64MB",
      "rawInputDataSize" : "0B",
      "rawInputPositions" : 0,
      "inputDataSize" : "115B",
      "inputPositions" : 1,
      "sumSquaredInputPositions" : 1.0,
      "getOutputCalls" : 820,
      "getOutputWall" : "33.97ms",
      "getOutputCpu" : "31.83ms",
      "getOutputAllocation" : "376.24kB",
      "outputDataSize" : "115B",
      "outputPositions" : 1,
      "physicalWrittenDataSize" : "0B",
      "blockedWall" : "0.00ns",
      "finishCalls" : 411,
      "finishWall" : "16.55ms",
      "finishCpu" : "15.69ms",
      "finishAllocation" : "94.47kB",
      "userMemoryReservation" : "0B",
      "revocableMemoryReservation" : "0B",
      "systemMemoryReservation" : "0B",
      "peakUserMemoryReservation" : "387.73kB",
      "peakSystemMemoryReservation" : "0B",
      "peakTotalMemoryReservation" : "387.73kB",
      "spilledDataSize" : "0B"
    }, {

== NO RELEASE NOTE ==

wenleix · 2020-03-02T22:10:41Z

cc @viczhang861 : Can this deprecate the tagged memory pool? 😃 (discussed in #14117)

viczhang861 · 2020-03-03T01:49:03Z

@wenleix Briefly went through change in this PR, it updates allocation when an operator completes. Tagged memory pool is updated whenever some bytes are reserved, which is more up-to-date (that is why tagged memory update is expensive but useful for debugging purpose).

viczhang861 · 2020-03-03T02:07:49Z

presto-main/src/main/java/com/facebook/presto/operator/OperationTimer.java

This is simpler to understand :)
!(trackOverallAllocation && THREAD_MX_BEAN_EX == null)
!(trackOperationAllocation && !trackOverallAllocation)

Move second check up

@viczhang861 Changed to

this.trackOverallAllocation = trackOverallAllocation; this.trackOperationAllocation = trackOperationAllocation; if (trackOverallAllocation) { checkArgument(THREAD_MX_BEAN_EX != null, "tracking allocation is not supported by this JVM"); } else { checkArgument(!trackOperationCpuTime, "tracking operation cpu time without tracking overall cpu time is not supported"); }

Much better. I restarted a failed test.

aweisberg

Everything looks good except for a nit regarding the naming "allocation timer enabled" and "per operator allocation timer enabled". The CPU version of this is a timer because it's measuring time.

This is tracking allocation bytes. I would suggest something like "tracking" or "measurement". Those aren't perfect either, but I can't think of anything better.

viczhang861 · 2020-03-03T21:15:31Z

Everything looks good except for a nit regarding the naming "allocation timer enabled" and "per operator allocation timer enabled". The CPU version of this is a timer because it's measuring time.

I think timer refers to an interval, not the specific measurement type. For example, OperationTiming also tracks calls.

arhimondr

LGTM % nits

arhimondr · 2020-03-03T21:49:16Z

presto-main/src/main/java/com/facebook/presto/execution/TaskManagerConfig.java

nit: maybe better allocationTracker? The word timer is used for the CPU because the CPU utilization is measured in "time" units (e.g.: milliseconds)

arhimondr · 2020-03-03T21:59:27Z

presto-main/src/main/java/com/facebook/presto/operator/OperationTimer.java

Currently Presto does not support any other VM's than Oracle JVM. How about just trying to cast ((com.sun.management.ThreadMXBean) THREAD_MX_BEAN) unconditionally inplace. In will throw a meaningful ClassCastException if for some very weird reson some other then com.sun.management.ThreadMXBean implementation is supplied.

arhimondr · 2020-03-03T22:00:02Z

presto-main/src/main/java/com/facebook/presto/operator/OperationTimer.java

tracking operator allocations without ...

mbasmanova · 2020-03-03T22:22:11Z

@aweisberg @arhimondr Ariel, Andrii, thank you for reviews. Comments addressed. CC: @rschlussel

aweisberg · 2020-03-04T22:29:33Z

presto-main/src/main/java/com/facebook/presto/operator/TaskContext.java

Still says timer

aweisberg · 2020-03-04T22:30:31Z

...k-base/src/main/java/com/facebook/presto/spark/execution/PrestoSparkTaskExecutorFactory.java

Timer here as well

aweisberg · 2020-03-04T22:30:40Z

...k-base/src/main/java/com/facebook/presto/spark/execution/PrestoSparkTaskExecutorFactory.java

And these two

aweisberg · 2020-03-04T22:30:54Z

...k-base/src/main/java/com/facebook/presto/spark/execution/PrestoSparkTaskExecutorFactory.java

aweisberg · 2020-03-04T22:31:44Z

presto-main/src/main/java/com/facebook/presto/operator/TaskContext.java

aweisberg

Found a few more timer instances. Otherwise LGTM.

mbasmanova · 2020-03-05T10:56:40Z

@aweisberg Ariel, thank you for review. I renamed the remaining timers.

aweisberg · 2020-03-05T18:23:13Z

LGTM! 🚢 it.

mbasmanova requested review from agrawaldevesh, arhimondr, bhhari, wenleix and yingsu00 March 2, 2020 21:45

wenleix requested a review from viczhang861 March 2, 2020 22:10

viczhang861 reviewed Mar 3, 2020

View reviewed changes

mbasmanova force-pushed the memory-allocation-tracking branch 2 times, most recently from 0bf4e22 to 99bfadc Compare March 3, 2020 15:11

viczhang861 approved these changes Mar 3, 2020

View reviewed changes

aweisberg suggested changes Mar 3, 2020

View reviewed changes

arhimondr approved these changes Mar 3, 2020

View reviewed changes

mbasmanova force-pushed the memory-allocation-tracking branch from 99bfadc to 8529410 Compare March 3, 2020 22:19

Add config properties to enable tracking memory allocations

0c8a829

mbasmanova force-pushed the memory-allocation-tracking branch from 8529410 to 6321132 Compare March 3, 2020 22:21

aweisberg reviewed Mar 4, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/operator/TaskContext.java Outdated

Copy link
Copy Markdown

Contributor

aweisberg Mar 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still says timer

aweisberg reviewed Mar 4, 2020

View reviewed changes

...k-base/src/main/java/com/facebook/presto/spark/execution/PrestoSparkTaskExecutorFactory.java Outdated

Copy link
Copy Markdown

Contributor

aweisberg Mar 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last one!

aweisberg reviewed Mar 4, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/operator/TaskContext.java Outdated

Copy link
Copy Markdown

Contributor

aweisberg Mar 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timer

aweisberg approved these changes Mar 4, 2020

View reviewed changes

mbasmanova added 5 commits March 5, 2020 05:55

Propagate flags to track allocation to OperationTimer

83c5628

Track allocations per operator

1715ef3

Add allocation info to OperatorStats

95fb2be

Refactor TestQueryStats

d1dce98

Add allocation into to DriverStats

5d2dc4c

mbasmanova added 2 commits March 5, 2020 05:55

Add allocation info to PipelineStats

30b8761

Add allocation info to Task/Stage/QueryStats

7caf111

mbasmanova force-pushed the memory-allocation-tracking branch from 6321132 to 7caf111 Compare March 5, 2020 10:55

mbasmanova merged commit 0e144a6 into prestodb:master Mar 5, 2020

mbasmanova deleted the memory-allocation-tracking branch March 5, 2020 18:50

Conversation

mbasmanova commented Mar 2, 2020

Uh oh!

wenleix commented Mar 2, 2020

Uh oh!

viczhang861 commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

viczhang861 commented Mar 3, 2020

Uh oh!

arhimondr 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!

mbasmanova commented Mar 3, 2020

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!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova commented Mar 5, 2020

Uh oh!

aweisberg commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viczhang861 commented Mar 3, 2020 •

edited

Loading