Skip to content

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

Open
nbali wants to merge 1 commit intomasterfrom
tweaks
Open

Ignore no-op load/save/delete actions properly#1
nbali wants to merge 1 commit intomasterfrom
tweaks

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

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" notifications@github.com):

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" notifications@github.com):

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" notifications@github.com):

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