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

Ignore no-op load/save/delete actions properly #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nbali
Copy link
Owner

@nbali nbali commented Oct 18, 2015

  • the goal is to avoid the small, but potentially frequently executed calls to save CPU

This change is Reviewable

@nbali
Copy link
Owner Author

nbali commented Oct 18, 2015

@fpeter8 pls, thx

@@ -122,6 +123,10 @@
for (S id: ids)
keymap.put(this.<T>makeKey(id), id);

if (keymap.isEmpty()) {
return Maps.newLinkedHashMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Collections.emptyMap();? Or you want it to be writable?

Copy link
Owner Author

Choose a reason for hiding this comment

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

TBH there is no proper consistency in the objectify itself, for example
some uses empty map, some new hashmap, but this method returned this
originally, so seemed like a good idea to keep it.
2015.10.19. 13:18 ezt írta ("Peter Farkas" [email protected]):

In src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java
#1 (comment):

@@ -122,6 +123,10 @@
for (S id: ids)
keymap.put(this.makeKey(id), id);

  •   if (keymap.isEmpty()) {
    
  •       return Maps.newLinkedHashMap();
    

Collections.emptyMap();? Or you want it to be writable?


Reply to this email directly or view it on GitHub
https://github.com/nbali/objectify/pull/1/files#r42359680.

@fpeter8
Copy link
Collaborator

fpeter8 commented Oct 19, 2015

please follow code conventions (one-line ifs don't have brackets)

@fpeter8 fpeter8 assigned nbali and unassigned fpeter8 Oct 19, 2015
@nbali
Copy link
Owner Author

nbali commented Oct 19, 2015

Funny part again, sometimes they dont, sometimes they do ;-)
2015.10.19. 13:20 ezt írta ("Peter Farkas" [email protected]):

please follow code conventions (one-line ifs don't have brackets)


Reply to this email directly or view it on GitHub
#1 (comment).

@fpeter8
Copy link
Collaborator

fpeter8 commented Oct 19, 2015

why did you modify LoadTypeImpl? It calls LoaderImpl (which contains the empty clause already). If this is such an improvement, why not SaveTypeImpl and DeleteTypeImpl?

@nbali
Copy link
Owner Author

nbali commented Oct 19, 2015

I wanted to avoid the unnecessary proxying, but you are right test should
include proxy creation as well
2015.10.19. 13:24 ezt írta ("Peter Farkas" [email protected]):

why did you modify LoadTypeImpl? It calls LoaderImpl (which contains the
empty clause already). If this is such an improvement, why not SaveTypeImpl
and DeleteTypeImpl?


Reply to this email directly or view it on GitHub
#1 (comment).

@nbali nbali force-pushed the tweaks branch 2 times, most recently from c4d5d90 to 4c65d23 Compare October 25, 2015 00:08
@nbali
Copy link
Owner Author

nbali commented Oct 25, 2015

@fpeter8 next round pls

* the goal is to avoid the small, but potentially frequently executed calls to save CPU
* removal of proxy creation in favor of forwarding list/map
@fpeter8
Copy link
Collaborator

fpeter8 commented Oct 26, 2015

I'll need to check the Round implementation to be sure now - nowUncached change is fine, the rest seems OK

@fpeter8
Copy link
Collaborator

fpeter8 commented Dec 17, 2015

@nbali FYI I've updated the master branch

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.

2 participants