Skip to content
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

Optimization of settings atrs to current thread. #32000

Closed
wants to merge 2 commits into from
Closed

Optimization of settings atrs to current thread. #32000

wants to merge 2 commits into from

Conversation

heckad
Copy link
Contributor

@heckad heckad commented Mar 20, 2019

No needed call slow threading.current_thread() on evry arg setting.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@mart-e
Copy link
Contributor

mart-e commented Apr 8, 2019

Hello,

Thank you for you patch but it seems you have not signed the CLA.
Could you add it so we can integrate your patch?
Regards

@Xavier-Do
Copy link
Contributor

Xavier-Do commented Apr 8, 2019

Hello

As far as I remember we took a quick look at the current_thread() implementation and it was quite light, but I may be wrong about that.
Could you explain why you consider it as slow?
After a quick testing, I have not more than twice the time of an access to a stored current_thread compared to the threading.current_thread() method:

import time
import threading 

ct = threading.current_thread()
def perfs(func, iter = 10000000):
    init = time.time()
    for i in range(0, iter):
        func()
    print('Finished %s in %s s' % (iter, (time.time()-init)))

def current_thread():
    return ct
    
perfs(threading.current_thread)
perfs(current_thread)

Results:

Finished 10000000 in 1.65838980675 s
Finished 10000000 in 0.879067182541 s

I tested with python2.7 and 3.7 and results are similar.

I don't mean that your suggestion is wrong but I'd like to understand why you consider that current_thread() is "slow" and if you have an example use case so that we can write a explanatory commit message.

@heckad
Copy link
Contributor Author

heckad commented Apr 9, 2019

Function threading.current_thread gets value from the dict. Good practice to get ref to the value and use this ref for manipulating them, not to get it from dict for manipulation. It`s just faster.

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 9, 2019
reuse current_thread result
No needed to call threading.current_thread() on evry arg setting
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 10, 2019
@mart-e
Copy link
Contributor

mart-e commented Apr 10, 2019

Thanks for the clarification. It makes no noticable difference for the performance but is a nice code cleaning.

@robodoo r+

@robodoo robodoo added r+ 👌 CI 🤖 Robodoo has seen passing statuses labels Apr 10, 2019
@robodoo
Copy link
Contributor

robodoo commented Apr 10, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@mart-e
Copy link
Contributor

mart-e commented Apr 10, 2019

robodoo rebase-ff

@robodoo
Copy link
Contributor

robodoo commented Apr 10, 2019

Merge method set to rebase and fast-forward

robodoo pushed a commit that referenced this pull request Apr 10, 2019
reuse current_thread result
No needed to call threading.current_thread() on evry arg setting

closes #32000

Signed-off-by: Martin Trigaux (mat) <mat@odoo.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 10, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 10, 2019
@heckad heckad deleted the patch-2 branch April 10, 2019 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants