You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/03/24 13:47:46 UTC

[GitHub] [beam] mosche opened a new pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

mosche opened a new pull request #17172:
URL: https://github.com/apache/beam/pull/17172


   Various improvements to getValue(fieldIdx) in RowWithGetters mentioned [here](https://issues.apache.org/jira/browse/BEAM-14166#discussion_r833602836)
   
   - Minimize memory overhead of cache using either a index lookup (array) or a single hash map if number of fields exceeds the initial hashmap capacity
   - The cache should be checked before calling a getter to avoid any potentially unnecessary conversion in the getter itself.
   [Nested rows](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java#L111) should be cached, otherwise the cache of such nested rows can't be leveraged.
   - Handling of collections / maps / iterables can be significantly improved by simply skipping the transform in all cases where getValue for members is the [identity transform](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java#L142).
   
   
   As mentioned in [BEAM-14166](https://issues.apache.org/jira/browse/BEAM-14166#discussion_r833602836), it would also be possible to (optionally) pre-calculate the required cache size and indices and store this as metadata in schema & field options. Though, this is probably a bit more controversial... If you'd like to evaluate this idea further I can do a prototype for Java Field/Bean schemas.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] reuvenlax commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

reuvenlax commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1080895751


   Benchmarks of this code are tricky to do, since the cost of codegen tends to dominate microbenchmarks.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] mosche edited a comment on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

mosche edited a comment on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1077772316


   @reuvenlax certainly a valid question and I'm happy to discuss what's worth caching. 
   
   Though, the key point here is, that there's already a cache in place for array, iterable and map types. Processing for these is currently not ideal. For instance, looking at an array field the following steps happen on `getValue`:
   
   1) Invoke getter, the generated byte code wraps the array in a list
   2) Then lazily transform the list elements (`Lists.transform`) and cache the result
   
   The problem is really on the 2nd access. Step 1) is repeated, but step 2) uses the cache discarding all the work of step 1).
   
   This PR mostly addresses some of the inconsistencies using the existing cache and reduces the overhead of the cache by choosing better fitted data structures to store cached values.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] mosche commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

mosche commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1079124818


   I started doing some jmh benchmarks for `RowWithGetter`. I'll follow up with next week on this...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] mosche commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

mosche commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1077649025


   R: @TheNeuralBit 
   R: @reuvenlax 
   R: @kennknowles 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] mosche commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

mosche commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1081574040

I've pushed my [benchmark code ](https://github.com/apache/beam/pull/17203) for reference, let me know if you have any suggestions. To prevent the issue you've mentioned @reuvenlax, I'm setting up a relatively large number of rows as JMH state before the actual benchmark invocation.

To establish a baseline, I'm looking at `master` first. [Here](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/apache/beam/3b5e1d607d30397abb27a119fc30c5d05a7b2d40/sdks/java/harness/jmh/RowWithGetters-master-cache-disabled.json,https://raw.githubusercontent.com/apache/beam/3b5e1d607d30397abb27a119fc30c5d05a7b2d40/sdks/java/harness/jmh/RowWithGetters-master-cache-enabled-lazy-init.json) are some initial results with some minimal changes to `RowWithGetters` to make these benchmarks meaningful:
- Disable caching for the first run.
- Change initialisation of the cache data structure to lazy init so associated costs are considered in the benchmark.

These numbers are certainly not very much in favour of the status quo.
I'm still iterating on improvements, but from what I've seen so far far there's lots that can be done.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] mosche commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

mosche commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1085981280


   I've drilled down into this a bit and I think I've got some interesting finding's to share @reuvenlax & @TheNeuralBit.
   
   Investigating a few approaches, I would to suggest to push field type based logic in `RowWithGetters` down into `FieldValueGetter`s to remove any branching and allow for better inlining, see [code & benchmark](https://github.com/apache/beam/compare/master...mosche:RowWithGetters-JMH-Getters-LogicPushDown).
   
   I also looked a bit into costs of caching, the picture isn't as clear there. The costs of initializing any data structure facilitating a cache is certainly high compared to the costs of calling getters. One finding though was that `TreeMap` didn't perform any worse than `HashMap`. Given the much lower memory footprint that might be a good pick then. Also, using [materialized Pojo lists](https://github.com/mosche/beam/blob/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/GetterBasedSchemaProvider.java#L178-L180) helped to improve the performance gain from caching (compared to lazy transforms using `Lists.transform`).
   
   On the other hand, I'm not sure what the original motivation for adding a field value cache in `RowWithGetters` was. Is it just about performance?
   
   Some visualizations for a few selected runs:
   - [master vs suggested approach (no caching)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-master-cache-disabled.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-disabled.json)
   - [master vs suggested approach (with caching)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-master-cache-HashMaps.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-TreeMap-lazy-lists.json)
   - [costs of caching (master)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-master-cache-disabled.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-master-cache-HashMaps.json)
   - [costs of caching (suggested approach, treemap)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-disabled.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-TreeMap-lazy-lists.json)
   - [costs of caching (suggested approach, hashmap)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-disabled.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-HashMap-lazy-lists.json)
   - [hashmap vs treemap if using materialized rather than lazy Pojo collections](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-HashMap-persisted-pojolists.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-TreeMap-persisted-pojolists.json)
   
   [![Screenshot 2022-04-01 at 14 49 46](https://user-images.githubusercontent.com/1401430/161275064-9f357215-6fb8-49a6-bb18-2c80b6bbf751.png)](https://jmh.morethan.io/?sources=https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-master-cache-HashMaps.json,https://raw.githubusercontent.com/mosche/beam/RowWithGetters-JMH-Getters-LogicPushDown/sdks/java/harness/jmh/jmh-pr-getters-cache-TreeMap-lazy-lists.json)
   
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] reuvenlax commented on pull request #17172: [BEAM-14166] Performance / Cache improvements to RowWithGetter

Posted by GitBox <gi...@apache.org>.

reuvenlax commented on pull request #17172:
URL: https://github.com/apache/beam/pull/17172#issuecomment-1077753040


   By default, getValue results in generated bytecode to fetch the value. Is this cache really more efficient than the generated bytecode?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org