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 2021/03/22 09:15:12 UTC

[GitHub] [incubator-doris] gaodayue opened a new pull request #5554: Add fmt library to speed up mysql text result serialization

gaodayue opened a new pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554


   ## Proposed changes
   
   This CL optimizes the performance of `MysqlResultWriter` in two aspects
   1. Use `fmt::format_int` to do int->string conversion, which is 6x faster than `sprintf` according to [this blog](https://www.zverovich.net/2020/06/13/fast-int-to-string-revisited.html)
   2. Reduce profiling overhead by measuring convert time at row batch level instead of row level
   
   This fixes #5553 . The `TupleConvertTime` dropped from 185.573ms to 27ms after this optimization.
   
   ## Types of changes
   
   What types of changes does your code introduce to Doris?
   _Put an `x` in the boxes that apply_
   
   - [ ] Bugfix (non-breaking change which fixes an issue)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
   - [ ] Documentation Update (if none of the other choices apply)
   - [x] Code refactor (Modify the code structure, format the code, etc...)
   
   ## Checklist
   
   _Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._
   
   - [x] I have created an issue on (Fix #ISSUE) and described the bug/feature there in detail
   - [ ] Compiling and unit tests pass locally with my changes
   - [ ] I have added tests that prove my fix is effective or that my feature works
   - [ ] If these changes need document changes, I have updated the document
   - [ ] Any dependent changes have been merged
   
   


-- 
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.

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 #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#discussion_r603733903



##########
File path: be/src/util/mysql_row_buffer.cpp
##########
@@ -106,15 +118,7 @@ int MysqlRowBuffer::push_tinyint(int8_t data) {
         return ret;
     }
 
-    int length = snprintf(_pos + 1, MAX_TINYINT_WIDTH + 2, "%d", data);
-
-    if (length < 0) {
-        LOG(ERROR) << "snprintf failed. data = " << data;
-        return length;
-    }
-
-    int1store(_pos, length);
-    _pos += length + 1;
+    _pos = add_int(data, _pos);

Review comment:
       is this means the convert will always success ？




-- 
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.

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] gaodayue commented on pull request #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
gaodayue commented on pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#issuecomment-804657626


   > Nice job! could you please update the docker env to add fmt lib.
   
   @HappenLee thanks for your reply! In fact I don't know the process about adding a new third party library, could you provide some helps? It would be better if we documented 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.

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 #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg commented on pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#issuecomment-805408563


   we should merge this pr after upgrading to gcc 10, then we need to update 1.3 docker env only.  I will update the docker env


-- 
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.

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 #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#discussion_r603733903



##########
File path: be/src/util/mysql_row_buffer.cpp
##########
@@ -106,15 +118,7 @@ int MysqlRowBuffer::push_tinyint(int8_t data) {
         return ret;
     }
 
-    int length = snprintf(_pos + 1, MAX_TINYINT_WIDTH + 2, "%d", data);
-
-    if (length < 0) {
-        LOG(ERROR) << "snprintf failed. data = " << data;
-        return length;
-    }
-
-    int1store(_pos, length);
-    _pos += length + 1;
+    _pos = add_int(data, _pos);

Review comment:
       is this means the cover will always success ？




-- 
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.

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] HappenLee commented on pull request #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
HappenLee commented on pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#issuecomment-804595075


   Nice job! could you please update the docker env to add fmt lib.


-- 
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.

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 #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#discussion_r600097205



##########
File path: thirdparty/build-thirdparty.sh
##########
@@ -655,6 +655,17 @@ build_croaringbitmap() {
     -DENABLE_ROARING_TESTS=OFF ..
     ${BUILD_SYSTEM} -j$PARALLEL && ${BUILD_SYSTEM} install
 }
+
+# fmt
+build_fmt() {
+    check_if_source_exist $FMT_SOURCE
+    cd $TP_SOURCE_DIR/$FMT_SOURCE
+    mkdir -p $BUILD_DIR && cd $BUILD_DIR
+    rm -rf CMakeCache.txt CMakeFiles/
+    $CMAKE_CMD -v -DBUILD_SHARED_LIBS=FALSE -DFMT_TEST=OFF -DFMT_DOC=OFF -DCMAKE_INSTALL_PREFIX=$TP_INSTALL_DIR ..

Review comment:
       ```suggestion
       $CMAKE_CMD -G "${GENERATOR}" -DBUILD_SHARED_LIBS=FALSE -DFMT_TEST=OFF -DFMT_DOC=OFF -DCMAKE_INSTALL_PREFIX=$TP_INSTALL_DIR ..
   ```




-- 
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.

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 merged pull request #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg merged pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554


   


-- 
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.

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] gaodayue commented on pull request #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
gaodayue commented on pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#issuecomment-805450059


   > we should merge this pr after upgrading to gcc 10, then we need to update 1.3 docker env only. I will update the docker env
   
   @yangzhg got it, thank you


-- 
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.

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 #5554: Add fmt library to speed up mysql text result serialization

Posted by GitBox <gi...@apache.org>.
yangzhg commented on a change in pull request #5554:
URL: https://github.com/apache/incubator-doris/pull/5554#discussion_r600097482



##########
File path: thirdparty/build-thirdparty.sh
##########
@@ -655,6 +655,17 @@ build_croaringbitmap() {
     -DENABLE_ROARING_TESTS=OFF ..
     ${BUILD_SYSTEM} -j$PARALLEL && ${BUILD_SYSTEM} install
 }
+
+# fmt
+build_fmt() {
+    check_if_source_exist $FMT_SOURCE
+    cd $TP_SOURCE_DIR/$FMT_SOURCE
+    mkdir -p $BUILD_DIR && cd $BUILD_DIR
+    rm -rf CMakeCache.txt CMakeFiles/
+    $CMAKE_CMD -v -DBUILD_SHARED_LIBS=FALSE -DFMT_TEST=OFF -DFMT_DOC=OFF -DCMAKE_INSTALL_PREFIX=$TP_INSTALL_DIR ..
+    make -j$PARALLEL && make install

Review comment:
       ```suggestion
       ${BUILD_SYSTEM} -j $PARALLEL && ${BUILD_SYSTEM} install
   ```




-- 
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.

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