Skip to content

Conversation

@SphtKr
Copy link
Collaborator

@SphtKr SphtKr commented Jan 23, 2015

Caches several reflection code paths that tended to get executed often with the same types, so should have a very positive effect on performance. Closes #23.

@SphtKr SphtKr added this to the 0.3.0 milestone Jan 23, 2015
@SphtKr
Copy link
Collaborator Author

SphtKr commented Jan 23, 2015

@csantero Interested in feedback on this and #24. Working to make it possible to support related object ID queries in #14 by making GetIdProperty() accessible, and doing some optimization along the way. Particularly wondering about my thread-safety (or possible lack thereof).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this return IDictionary instead of Dictionary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, probably. I'll change that.

@SphtKr
Copy link
Collaborator Author

SphtKr commented Jan 28, 2015

Okay, I'm done with the big refactoring pass. @csantero please have a look.

Notes:

  • I found that GetPropertyMap shouldn't really be a public interface; the fact that I built a map was an implementation detail/optimization strategy. What clients need is methods that provide information on the properties, types, and corresponding keys. Internally, those methods use a map to cache reflection information.
  • It's somewhat irritating that we can't require a constructor via the interface...so we can't require implementers to accept a IPluralizationService as a constructor argument. Yes, ultimately it's up to them how to populate that property, but it still seems like part of the contract is missing.
  • IsMany became IsSerializedAsMany
  • GetSingleType became GetElementType, which no longer works for non-many types, and I left a private convenience method in JsonApiFormatter that replicates the old behavior but calls the IModelManager methods. I'm not happy with the name GetElementType but am not sure what else to call it.
  • I'm thinking ahead to other uses for IModelManagers...besides using it in EnableFilteringAttribute and MetadataManager. For example, I think with a few more methods it should be possible to build a model generator for Ember Data (!). I had a class that used to do this, but it's horribly broken by now. I really liked it, even if I had to tweak the output, it made model refactors so much easier not to have to sync changes to the client code by hand. It also might be valuable to optionally let users pre-register their model objects to build the cached reflection data at boot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this say "Inverse of GetJsonKeyForProperty"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, thanks

@csantero csantero mentioned this pull request Jan 28, 2015
@csantero
Copy link
Collaborator

@SphtKr This is really good. I'm excited about the peformance improvements this will offer (although we don't have any performance tests to prove this). I've submitted a PR to clean up those constructors a bit.

Does ModelManager need to have a parameterless constructor at all? Can we just remove that and also the equivalent parameterless constructor on JsonApiFormatter? Then the user can't not provide a pluralization service.

@SphtKr
Copy link
Collaborator Author

SphtKr commented Jan 28, 2015

I suppose not, it's left over from the singleton pattern in the case of ModelManager I suppose. And yes now there's no reason to have the parameter-less constructor on JsonApiFormatter either. That's good too, because I don't want it to be easy for the user to use the default pluralizer, since it's so naïve, they should have to choose it.

…onstructors. Added caching to `IsSerializedAsMany` and `GetElementType`. Made `ModelManager` a little more subclassing-friendly.
SphtKr added a commit that referenced this pull request Jan 29, 2015
@SphtKr SphtKr merged commit 98287c0 into master Jan 29, 2015
@csantero csantero deleted the cache-propmaps-issue23 branch July 18, 2015 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve performance when introspecting model classes

3 participants