New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Circular buffer should use conditional statement instead of modulo #7890

Merged
merged 2 commits into from Sep 27, 2018

Conversation

Projects
None yet
8 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Aug 24, 2018

Description

  1. Resolves: #7701
  2. Moved Circular buffer test to platform folder

Pull request type

[X] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@cmonr cmonr requested a review from ARMmbed/mbed-os-core Aug 25, 2018

@cmonr cmonr added the needs: review label Aug 25, 2018

@0xc0170

AStyle CI review (fix needed)

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:cb_issue_7701 branch from 9f3b838 to 4e263b1 Aug 27, 2018

AStyle fix done

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Sep 12, 2018

@SenRamakri @geky - Please review

@deepikabhavnani deepikabhavnani requested a review from kegilbert Sep 14, 2018

@kegilbert

LGTM!

@cmonr cmonr added needs: CI and removed needs: review labels Sep 20, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 26, 2018

...huh. Kinda surprised to find that we're using modulo, since I would expect masking arythmetic to be used for this kind of rollover. Still, an if statement is a bit of an improvement.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 26, 2018

Build : SUCCESS

Build number : 3158
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7890/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@geky

This comment has been minimized.

Member

geky commented Sep 26, 2018

...huh. Kinda surprised to find that we're using modulo, since I would expect masking arythmetic to be used for this kind of rollover. Still, an if statement is a bit of an improvement.

In this case BufferSize is a compile-time constant (template). So the compiler can trivially turn it into a mask when it's a power of 2.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 26, 2018

06:46:19 FATAL: java.io.IOException: Unexpected termination of the channel

Wow. It's been a while since we last saw something like this.

/morph export

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 26, 2018

In this case BufferSize is a compile-time constant (template). So the compiler can trivially turn it into a mask when it's a power of 2.

Gotcha. Is the compiler still able to do the optimization with this PR change?

@geky

This comment has been minimized.

Member

geky commented Sep 26, 2018

Nope: https://godbolt.org/z/aYf-MO

Though note how much worse it gets when you change SIZE to a non-power of two.

Note that is we really wanted to, we could have a template specialization for powers of two, and a template specialization for non-powers of two. But this may take much much more effort than it's really worth.

@pan-

This comment has been minimized.

Member

pan- commented Sep 26, 2018

@cmonr No the compiler doesn't apply the optimisation: https://godbolt.org/z/yEKolO .
However the proposed change is much better when the buffer size is not a power of 2: https://godbolt.org/z/uDriqd

Edit: As @geky pointed out there's performance salvation with partial specialisation however it'll be maintenance hell for uninitiated programmers.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 27, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Sep 27, 2018

@cmonr cmonr merged commit 4403a56 into ARMmbed:master Sep 27, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(-0.09%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 554 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9129 cycles (-21 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Sep 27, 2018

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:cb_issue_7701 branch Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment