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

[IMP] remove useless locking from update() #9

Merged
merged 1 commit into from Nov 14, 2014

Conversation

clonedagain
Copy link

Locking shouldn't be needed here because the data we read is immutable.
Supersedes https://code.launchpad.net/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls/+merge/216353
"If I'm not mistaken, locking in printer.printer.update() is not needed, and the way it's written does not protect us from concurrent changes anyway :

  • if what we read is immutable than the value is copied and locking is unnecessary
  • if it's mutable then we're getting a "reference" to the shared object, and should keep the lock until we're completely done with it

From my quick code survey, the variables seem to contain a boolean and a float so I guess no locking is needed.

update() seems to be called pretty often (all the CRUD method call it) so It may help a bit to remove the contention."

Locking shouldn't be needed here because the data we read is immutable
@clonedagain clonedagain force-pushed the 7.0-no-lock-in-update-1308635-ls branch from d2d5c45 to 06f8bdf Compare October 24, 2014 16:50
@guewen
Copy link
Member

guewen commented Nov 14, 2014

👍

1 similar comment
@pedrobaeza
Copy link
Member

👍

pedrobaeza added a commit that referenced this pull request Nov 14, 2014
@pedrobaeza pedrobaeza merged commit 3013c91 into OCA:7.0 Nov 14, 2014
@clonedagain clonedagain deleted the 7.0-no-lock-in-update-1308635-ls branch November 14, 2014 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants