Skip to content

Remove extra checks from loop#9288

Open
spirinvladimir wants to merge 1 commit intotensorflow:masterfrom
spirinvladimir:master
Open

Remove extra checks from loop#9288
spirinvladimir wants to merge 1 commit intotensorflow:masterfrom
spirinvladimir:master

Conversation

@spirinvladimir
Copy link

@spirinvladimir spirinvladimir commented Sep 23, 2020

Description

Loop with 2 conditions inside replaced with 3 loops without such conditions inside.

Type of change

Changes improve time complexity.

Note: Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update
  • TensorFlow 2 migration
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • A new research paper code implementation
  • Other (performance)

Tests

Is any performance tests required?

Test Configuration:

Checklist

Changes improve time complexity.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@spirinvladimir
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jaeyounkim jaeyounkim added the models:research models that come under research directory label Sep 25, 2020
Copy link
Contributor

@prabhukaliamoorthi prabhukaliamoorthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change.

This loop is not critical to performance so I won't duplicate the loop to reduce comparisons.

@spirinvladimir
Copy link
Author

@prabhukaliamoorthi I'm glad that you saw duplication of codebase. This was a trump sorry 😄 . Any way there is a general solution to move common parts to new function. Should I do it? I'm asking cause not clear how you calculate performance and what is critical? I calculate performance by by time complexity.

@prabhukaliamoorthi
Copy link
Contributor

In general the loop you are optimizing is executed once for each token in the text. That particular code takes 1 - 2% of the overall inference time when profiled on an Android device. So optimizing it would not make a noticeable difference to the inference time. In such situation I prefer readability over performance.

@spirinvladimir
Copy link
Author

Should I move code duplication to a function? You might want suggest a name to improve readability.
Code duplication isn't show stopper. It can be refactored.
About performance improvement:
Should not we accept any performance improvement even % of improvement is low?

I'm Vladimir Spirin. I as a part of open-source community want to be reviewed by other developers. Current code base have to be improved. Our code base mentioned at public news and I we should keep good code quality.

@prabhukaliamoorthi
Copy link
Contributor

Sorry for the delayed reply. I need to move couple of internal changes to the github repo. So I am delaying this cl. One other thing is all the tests should pass.

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

Labels

cla: yes models:research models that come under research directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants