TaskModel + AccumInfo refactor for memory optimisation #1540
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sayed Bilal Bari <sbari@nvidia.com>
sayedbilalbari
requested review from
amahussein,
cindyyuanjiang and
parthosa
Signed-off-by: Sayed Bilal Bari <sbari@nvidia.com>
Signed-off-by: Sayed Bilal Bari <sbari@nvidia.com>
parthosa
reviewed
Feb 11, 2025
| storeIfNonZero(EXEC_DESERIALIZE_TIME, metrics.executorDeserializeTime) | ||
| storeIfNonZero(EXEC_DESERIALIZE_CPU_TIME, metrics.executorDeserializeCpuTime) | ||
| storeIfNonZero(EXEC_RUN_TIME, metrics.executorRunTime) | ||
| storeIfNonZero(EXEC_CPU_TIME, metrics.executorCpuTime) |
There was a problem hiding this comment.
nit: Can we retain some of the comments that were present in the original metrics? (e.g. nanoseconds)
| output_recordsWritten: Long) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit: Is there an extra line before the object?
|
|
||
| var taskMetrics = immutable.IntMap.empty[Long] | ||
|
|
||
| def storeIfNonZero(field: Int, value: Long): Unit = |
There was a problem hiding this comment.
Can we include{} instead of inlined statements and a new line after the function definition?
|
This PR is outdated due to the changes in the incoming PR - #1543. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves for #815
Changes -
This PR introduces the following refactors -
TaskModelis a class that maintains details related to a TaskInfo. On observation the data related to TaskModel was pretty sparse( metrics like bytesread, byteswritten are only present in a few ). Hence updating the structure to maintain the long value metrics in a map and storing them only when required non zero.Improvements -
This refactor improves the shallow memory usage by ~35%. Attached are the screenshots from the peak heap dump -
Old heap dump -
Post refactor dump -
Testing -
This has been tested against a databricks event log file and improves the minimum heap required by 15% down from 12GB to 14GB
Although the heap analysis show the peak usage going down till 9GB, the reason it still needs the extra heap buffer is the usage of LongMap and IntMap which do reallocation where it double the current Buffer size even though it does not need it.
Optimisation -
Optimisation of this can be using 3rd party Map implementations which can further leverage the remaining heap space
Update on further testing
On further testing and discussion, this result is not very consistent though. Mainly because of the way LongMap and IntMap function ( over allocation of fixed buffer memory ).
A better long term solution would be to get rid of TaskModel altogether and leverage the Accumulable to get this information. This is just used to get the Task level metrics ( min, med, max, sum) and all of these can be retrieved from the accumulable information that we are storing.