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/02/22 11:24:20 UTC
[GitHub] [incubator-doris] zuochunwei opened a new pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
zuochunwei opened a new pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193
# Proposed changes
i have tested final keyword's effect, the result shows
1. call virtual member functions through derived class instance will more efficient if the function is marked as final, even if i use variable for switch, it's more efficient than virtual function vall
2. tidy codes, some member data is not necessary, so i deleted it
here is the test code:
`
uint64_t get_time()
{
timespec end;
clock_gettime(CLOCK_MONOTONIC, &end);
return (end.tv_sec) * 1000L * 1000L * 1000L + (end.tv_nsec);
}
class Base {
public:
Base() : type(0) {}
virtual ~Base() {}
virtual void f(int i) { ++x; }
int x = 0;
int type = 0;
};
int a[10] = {1, 3, 4, 8, 9, 2, 6, 7, 5, 0};
class Derived : public Base {
public:
Derived() { type = 1;}
void f(int i) /*final*/ { j += a[i]; }
int j = 0;
};
static void Test1() {
uint64_t start = get_time();
Derived* d = new Derived;
Base* b = d;
for (int x = 0; x < 1000; ++x)
{
for (int i = 0; i < 10; ++i) b->f(i);
}
uint64_t end = get_time();
std::cout << "j=" << d->j << " time:" << (end - start) << std::endl;
}
static void Test2() {
uint64_t start = get_time();
Derived* d = new Derived;
for (int x = 0; x < 1000; ++x)
{
for (int i = 0; i < 10; ++i) d->f(i);
}
uint64_t end = get_time();
std::cout << "j=" << d->j << " time:" << (end - start) << std::endl;
}
int main()
{
Test1();
Test2();
return 0;
}
`
Issue Number: close #xxx
## Problem Summary:
Describe the overview of changes.
## 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] [incubator-doris] zuochunwei commented on a change in pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
zuochunwei commented on a change in pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#discussion_r812534442
##########
File path: be/src/olap/rowset/segment_v2/binary_dict_page.cpp
##########
@@ -207,71 +200,54 @@ Status BinaryDictPageDecoder::init() {
// copy the codewords into a temporary buffer first
// And then copy the strings corresponding to the codewords to the destination buffer
TypeInfo* type_info = get_scalar_type_info(OLAP_FIELD_TYPE_INT);
-
RETURN_IF_ERROR(ColumnVectorBatch::create(0, false, type_info, nullptr, &_batch));
- _data_page_decoder.reset(_bit_shuffle_ptr = new BitShufflePageDecoder<OLAP_FIELD_TYPE_INT>(_data, _options));
+ auto status = (new (&_bit_shuffle) BitShufflePageDecoder<OLAP_FIELD_TYPE_INT>(_data, _options))->init();
+ if (!status.ok()) return status;
Review comment:
because too long if write RETURN_IF_ERROR((new (&_bit_shuffle) BitShufflePageDecoder<OLAP_FIELD_TYPE_INT>(_data, _options))->init())
--
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] [incubator-doris] wangbo commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
wangbo commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048730537
The optimization of virtual function scheduling overhead is usually reflected in the processing of each row of data.
But PageDecoder's method is called in the processing of a batch, so it's normal to see no noticeable improvement.
If there are no other benefits, such as abstraction, I recommend not changing existing interfaces.
--
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] [incubator-doris] yangzhg commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
yangzhg commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048417106
what is the test result? how much does `final` faster than non-final ?
--
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] [incubator-doris] zbtzbtzbt commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword and reuse BinaryDictPageDecoder's members
Posted by GitBox <gi...@apache.org>.
zbtzbtzbt commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1060263635
I add a run-able link for `final` speed test. https://godbolt.org/z/v3MbqTrKf
--
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] [incubator-doris] morningman commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
morningman commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048722037
This PR does not only "add final keywords", but also did some "memory reuse" improvement. Better to describe it in PR comment too.
--
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] [incubator-doris] wangbo commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
wangbo commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048732445
For the optimization of memory reuse， I suggest making another independent 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@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] [incubator-doris] yangzhg commented on a change in pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#discussion_r812530938
##########
File path: be/src/olap/rowset/segment_v2/binary_dict_page.cpp
##########
@@ -207,71 +200,54 @@ Status BinaryDictPageDecoder::init() {
// copy the codewords into a temporary buffer first
// And then copy the strings corresponding to the codewords to the destination buffer
TypeInfo* type_info = get_scalar_type_info(OLAP_FIELD_TYPE_INT);
-
RETURN_IF_ERROR(ColumnVectorBatch::create(0, false, type_info, nullptr, &_batch));
- _data_page_decoder.reset(_bit_shuffle_ptr = new BitShufflePageDecoder<OLAP_FIELD_TYPE_INT>(_data, _options));
+ auto status = (new (&_bit_shuffle) BitShufflePageDecoder<OLAP_FIELD_TYPE_INT>(_data, _options))->init();
+ if (!status.ok()) return status;
Review comment:
why not use `RETURN_IF_ERROR()`
--
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] [incubator-doris] zuochunwei commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
zuochunwei commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048421408
> what is the test result? how much does `final` faster than non-final ?
call non-final virtual function's cost by subclass instance is same to virtual function calling
when add final key-word, call virtual function by subclass instance is same to normal function calling
the test result: 18000 VS 1000
--
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] [incubator-doris] zenoyang commented on pull request #8193: [refactor](storage)(olap) improve pageDecoder using final keyword
Posted by GitBox <gi...@apache.org>.
zenoyang commented on pull request #8193:
URL: https://github.com/apache/incubator-doris/pull/8193#issuecomment-1048718553
### Q2.1 flat table query:
```sql
SELECT
sum(LO_REVENUE),
(LO_ORDERDATE DIV 10000) AS year,
P_BRAND
FROM lineorder_flat
WHERE P_CATEGORY = 'MFGR#12' AND S_REGION = 'AMERICA'
GROUP BY
year,
P_BRAND
ORDER BY
year,
P_BRAND;
```
result:
```
before PR test:
- Total: 1s287ms
- BlockLoadTime: 6s779ms
- BlockLoadTime: 6s677ms
- BlockLoadTime: 6s699ms
- BlockLoadTime: 6s787ms
- BlockLoadTime: 6s801ms
- BlockLoadTime: 6s759ms
- BlockLoadTime: 6s729ms
- BlockLoadTime: 6s772ms
avg: 6750.375ms
after PR test:
- Total: 1s353ms
- BlockLoadTime: 6s897ms
- BlockLoadTime: 6s838ms
- BlockLoadTime: 6s850ms
- BlockLoadTime: 6s881ms
- BlockLoadTime: 6s965ms
- BlockLoadTime: 6s878ms
- BlockLoadTime: 6s941ms
- BlockLoadTime: 6s848ms
avg: 6887.25ms
```
### Q2.2 flat table query:
```sql
SELECT
sum(LO_REVENUE),
(LO_ORDERDATE DIV 10000) AS year,
P_BRAND
FROM lineorder_flat
WHERE P_BRAND >= 'MFGR#2221' AND P_BRAND <= 'MFGR#2228' AND S_REGION = 'ASIA'
GROUP BY
year,
P_BRAND
ORDER BY
year,
P_BRAND;
```
result:
```
before PR test:
- Total: 1s408ms
- BlockLoadTime: 7s622ms
- BlockLoadTime: 7s674ms
- BlockLoadTime: 7s714ms
- BlockLoadTime: 7s552ms
- BlockLoadTime: 7s596ms
- BlockLoadTime: 7s752ms
- BlockLoadTime: 7s870ms
- BlockLoadTime: 7s782ms
avg: 7695.25ms
after PR test:
- Total: 1s449ms
- BlockLoadTime: 7s738ms
- BlockLoadTime: 7s774ms
- BlockLoadTime: 7s654ms
- BlockLoadTime: 7s783ms
- BlockLoadTime: 7s751ms
- BlockLoadTime: 7s768ms
- BlockLoadTime: 7s764ms
- BlockLoadTime: 7s825ms
avg: 7757.125ms
```
### Q2.3 flat table query:
```sql
SELECT
sum(LO_REVENUE),
(LO_ORDERDATE DIV 10000) AS year,
P_BRAND
FROM lineorder_flat
WHERE P_BRAND = 'MFGR#2239' AND S_REGION = 'EUROPE'
GROUP BY
year,
P_BRAND
ORDER BY
year,
P_BRAND;
```
result:
```
before PR test:
- Total: 1s217ms
- BlockLoadTime: 6s47ms
- BlockLoadTime: 6s138ms
- BlockLoadTime: 6s360ms
- BlockLoadTime: 6s399ms
- BlockLoadTime: 5s877ms
- BlockLoadTime: 6s70ms
- BlockLoadTime: 6s293ms
- BlockLoadTime: 6s233ms
avg: 6308.75ms
after PR test:
- Total: 1s211ms
- BlockLoadTime: 6s362ms
- BlockLoadTime: 6s276ms
- BlockLoadTime: 6s277ms
- BlockLoadTime: 6s347ms
- BlockLoadTime: 6s363ms
- BlockLoadTime: 6s328ms
- BlockLoadTime: 6s299ms
- BlockLoadTime: 6s622ms
avg: 6359.25ms
```
--
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