You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/04 19:40:42 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #8464: Optimize single element ArrayList creation
Jackie-Jiang opened a new pull request, #8464:
URL: https://github.com/apache/pinot/pull/8464
Replace `new ArrayList<>(Collections.singletonList(e))` with a util function
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1087975492
This appears to amount to a slight regression, at least it's in the noise, on a tiny baseline cost:
```java
@State(Scope.Benchmark)
public class OneElementList {
@Param("*")
String value;
@Benchmark
public List<String> singletonList() {
return new ArrayList<>(Collections.singletonList(value));
}
@Benchmark
public List<String> optimized() {
List<String> list = new ArrayList<>(1);
list.add(value);
return list;
}
}
```
```
Benchmark (value) Mode Cnt Score Error Units
OneElementList.optimized * avgt 5 6.138 ± 0.127 ns/op
OneElementList.singletonList * avgt 5 5.224 ± 0.541 ns/op
```
With the change 48 bytes is allocated per list, before it was 72 bytes. Maybe it's worthwhile for those 24 bytes.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1087979495
The issue here is that we need to perform a recursive check for expression override.
If not changing this to a mutable single array list, then we need to check if the list is immutable which may bring in more overhead.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1087986944
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8464](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (72e3c07) into [master](https://codecov.io/gh/apache/pinot/commit/5e56b9a155b9f5d7ba3cd7214c15b3f1c697a351?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e56b9a) will **decrease** coverage by `0.01%`.
> The diff coverage is `56.25%`.
```diff
@@ Coverage Diff @@
## master #8464 +/- ##
============================================
- Coverage 70.70% 70.69% -0.02%
+ Complexity 4285 4281 -4
============================================
Files 1664 1665 +1
Lines 87342 87347 +5
Branches 13227 13227
============================================
- Hits 61757 61750 -7
- Misses 21296 21302 +6
- Partials 4289 4295 +6
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.21% <25.00%> (+0.08%)` | :arrow_up: |
| integration2 | `25.78% <25.00%> (-0.35%)` | :arrow_down: |
| unittests1 | `67.03% <56.25%> (-0.07%)` | :arrow_down: |
| unittests2 | `14.13% <0.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...t/core/query/selection/SelectionOperatorUtils.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zZWxlY3Rpb24vU2VsZWN0aW9uT3BlcmF0b3JVdGlscy5qYXZh) | `93.48% <20.00%> (ø)` | |
| [...ot/common/request/context/RequestContextUtils.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L1JlcXVlc3RDb250ZXh0VXRpbHMuamF2YQ==) | `72.81% <50.00%> (-0.23%)` | :arrow_down: |
| [...pql/parsers/PinotQuery2BrokerRequestConverter.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QaW5vdFF1ZXJ5MkJyb2tlclJlcXVlc3RDb252ZXJ0ZXIuamF2YQ==) | `89.18% <100.00%> (ø)` | |
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `87.55% <100.00%> (ø)` | |
| [...ain/java/org/apache/pinot/spi/utils/ListUtils.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTGlzdFV0aWxzLmphdmE=) | `100.00% <100.00%> (ø)` | |
| [...nt/local/startree/v2/store/StarTreeDataSource.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZURhdGFTb3VyY2UuamF2YQ==) | `40.00% <0.00%> (-13.34%)` | :arrow_down: |
| [...ore/query/scheduler/resources/ResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvcmVzb3VyY2VzL1Jlc291cmNlTWFuYWdlci5qYXZh) | `85.71% <0.00%> (-10.72%)` | :arrow_down: |
| [...a/org/apache/pinot/common/utils/ServiceStatus.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VydmljZVN0YXR1cy5qYXZh) | `60.00% <0.00%> (-7.15%)` | :arrow_down: |
| [...core/startree/operator/StarTreeFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9vcGVyYXRvci9TdGFyVHJlZUZpbHRlck9wZXJhdG9yLmphdmE=) | `87.41% <0.00%> (-5.60%)` | :arrow_down: |
| [...ot/segment/local/startree/OffHeapStarTreeNode.java](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9PZmZIZWFwU3RhclRyZWVOb2RlLmphdmE=) | `72.22% <0.00%> (-5.56%)` | :arrow_down: |
| ... and [28 more](https://codecov.io/gh/apache/pinot/pull/8464/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5e56b9a...72e3c07](https://codecov.io/gh/apache/pinot/pull/8464?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang closed pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang closed pull request #8464: Optimize single element ArrayList creation
URL: https://github.com/apache/pinot/pull/8464
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1088001276
Since the perf number is so close, probably not worth the effort of maintaining the code
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] FelixGV commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
FelixGV commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1087952792
Would it not be more optimal to return just `Collections.singletonList(e)` rather than wrapping the one element in an `ArrayList`? Memory-wise, the singleton list should have less memory overhead since it doesn't carry an array. The singleton list, however, is immutable, and will throw `UnsupportedOperationException` if you try to add more items into it, which may be either good or bad, depending on the use case.
If an immutable single element list is what you're looking for, then I would think singletonList is ideal. Moreover, in cases where you're passing a constant into the list (e.g. when you're passing `"*"` or `ExpressionContext.forIdentifier("*")`) then you could allocate that statically and keep reusing the same instance, which could be even more efficient.
The above may or may not make sense in this context, so take it with a grain of salt. I am always curious about performance-related work in other projects, to see if I can leverage them elsewhere, which is why I looked at this PR (: ...
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on pull request #8464: Optimize single element ArrayList creation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #8464:
URL: https://github.com/apache/pinot/pull/8464#issuecomment-1087962081
Do we have any measurements which indicate this is worthwhile? This sort of thing can be surprising.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org