You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/06/24 02:06:55 UTC
[GitHub] [doris] mrhhsg opened a new pull request, #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
mrhhsg opened a new pull request, #10386:
URL: https://github.com/apache/doris/pull/10386
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
`std::iota` is faster than for loop.
## Checklist(Required)
1. Does it affect the original behavior: (Yes/No/I Don't know)
2. Has unit tests been added: (Yes/No/No Need)
3. Has document been added or modified: (Yes/No/No Need)
4. Does it need to update dependencies: (Yes/No)
5. Are there any changes that cannot be rolled back: (Yes/No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165141911
PR approved by at least one committer and no changes requested.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] mrhhsg commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
mrhhsg commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165430808
> #9111 How about test this pr? Let's see which is faster.
@wangbo Great! It is the same with `std::iota`
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906043333
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
> There are two reasons why this loop can not be vectorized:
>
> 1. `nrows_read` is reference
> 2. `nrows_read` is unsigned
These reasons are not the root causes.
```cpp
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
In above snippet, if you change the type of `nrows_read` from `uint32_t` to `uint64_t`, you will find compiler vectorizes the loop.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906045375
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
I think the way #9109 which @wangbo used to analyze is more straightforward.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906043333
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
> There are two reasons why this loop can not be vectorized:
>
> 1. `nrows_read` is reference
> 2. `nrows_read` is unsigned
These reasons are not the root causes.
```cpp
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
In above snippet, if you change the type of `nrows_read` from `uint32_t` to `uint64_t`, you will find compiler vectorizes the loop.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905863804
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
This comment is wrong, compiler vectorizes both loops.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906043333
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
> There are two reasons why this loop can not be vectorized:
>
> 1. `nrows_read` is reference
> 2. `nrows_read` is unsigned
These reasons are not the root cause.
```cpp
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
In above snippet, if you change the type of `nrows_read` from `uint32_t` to `uint64_t`, you will find compiler vectorizes the loop.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906045375
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
I think the way #9111 which @wangbo used to analyze is more straightforward.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] mrhhsg commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
mrhhsg commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905877588
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
I tested with the code below(compile command: ` g++ test.cc -O3 -fopt-info-vec-all`:
1. `nrows_read` as reference:
```c++
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
got:
<img width="1223" alt="image" src="https://user-images.githubusercontent.com/1179834/175503549-988a66e8-1ebc-42bc-a4bc-2e241ad165ee.png">
2. `nrows_read` as value and signed.
```c++
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, int32_t nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
<img width="1220" alt="image" src="https://user-images.githubusercontent.com/1179834/175503955-171ece10-0d2c-4d66-b7c6-5ab0e0fb012d.png">
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangbo commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
wangbo commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165411960
https://github.com/apache/doris/pull/9111
How about test this pr? Let's see which is faster.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] mrhhsg commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
mrhhsg commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165334068
> Hi @mrhhsg , the underlying implementation of `std::iota` is also a loop. Why does this modification take effect?
>
> ```c++
> template <class _ForwardIterator, class _Tp>
> inline _LIBCPP_INLINE_VISIBILITY
> void
> iota(_ForwardIterator __first, _ForwardIterator __last, _Tp __value_)
> {
> for (; __first != __last; ++__first, (void) ++__value_)
> *__first = __value_;
> }
> ```
Maybe the original for-loop cannot be vectorized.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] mrhhsg commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
mrhhsg commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905868074
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
There are two reasons why this loop can not be vectorized:
1. `nrows_read` is reference
2. `nrows_read` is unsigned
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906043333
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
> There are two reasons why this loop can not be vectorized:
>
> 1. `nrows_read` is reference
> 2. `nrows_read` is unsigned
These reasons are not the root causes.
```cpp
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
In above snippet, if you change the type of `nrows_read` from `uint32_t` to `uint64_t`, you will find compiler vectorizes the loop.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165141936
PR approved by anyone and no changes requested.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905871733
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
I compared both assembly code generated by GCC 12.1 + O3, the difference between them is subtle. Compiler optimizes both loops by SSE registers.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905875127
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
It seems the cycle cost consumed by different arithmetical instructions makes improvement.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165197950
Hi @mrhhsg , the underlying implementation of `std::iota` is also a loop. Why does this modification take effect?
```cpp
template <class _ForwardIterator, class _Tp>
inline _LIBCPP_INLINE_VISIBILITY
void
iota(_ForwardIterator __first, _ForwardIterator __last, _Tp __value_)
{
for (; __first != __last; ++__first, (void) ++__value_)
*__first = __value_;
}
```
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906043333
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
> There are two reasons why this loop can not be vectorized:
>
> 1. `nrows_read` is reference
> 2. `nrows_read` is unsigned
These reasons are not the root causes.
```cpp
#include <string>
#include <vector>
std::vector<uint32_t> _block_rowids;
void func(uint32_t range_from, uint32_t range_to, uint32_t& nrows_read) {
uint32_t* data = _block_rowids.data();
for (uint32_t rid = range_from; rid < range_to; rid++) {
data[nrows_read++] = rid;
}
}
int main(int argc, char* argv[]) {
uint32_t nrows_read = 0;
_block_rowids.resize(4096);
func(100, 2000, nrows_read);
return 0;
}
```
In above snippet, if you change the type of `nrows_read` from `uint32_t` to `uint64_t`, you will find compiler vectorizes the loop.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905875127
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
It seems the cycle cost consumed by different arithmetical instructions makes improvement.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r905875127
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
The improvement here seems the cycle cost consumed by different arithmetical instructions.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165467828
PR approved by at least one committer and no changes requested.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] yiguolei merged pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
yiguolei merged PR #10386:
URL: https://github.com/apache/doris/pull/10386
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on PR #10386:
URL: https://github.com/apache/doris/pull/10386#issuecomment-1165336946
> > Hi @mrhhsg , the underlying implementation of `std::iota` is also a loop. Why does this modification take effect?
> > ```c++
> > template <class _ForwardIterator, class _Tp>
> > inline _LIBCPP_INLINE_VISIBILITY
> > void
> > iota(_ForwardIterator __first, _ForwardIterator __last, _Tp __value_)
> > {
> > for (; __first != __last; ++__first, (void) ++__value_)
> > *__first = __value_;
> > }
> > ```
>
> Maybe the original for-loop cannot be vectorized.
I think it is better to provide some evidences to prove it.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #10386: [improvement]Use std::iota to set values of _block_rowids in SegmentIterator::_read_columns_by_index
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #10386:
URL: https://github.com/apache/doris/pull/10386#discussion_r906073521
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -872,9 +872,11 @@ Status SegmentIterator::_read_columns_by_index(uint32_t nrows_read_limit, uint32
_read_columns(_first_read_column_ids, _current_return_columns, rows_to_read));
_cur_rowid += rows_to_read;
if (set_block_rowid) {
- for (uint32_t rid = range_from; rid < range_to; rid++) {
- _block_rowids[nrows_read++] = rid;
- }
+ // Here use std::iota is better performance than for-loop, maybe for-loop is not vectorized
Review Comment:
Why not just change the interface `SegmentIterator::_read_columns_by_index` to
```cpp
Status SegmentIterator::_read_columns_by_index(uint64_t nrows_read_limit, uint64_t& nrows_read,
bool set_block_rowid) {
```
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org