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

Added caching to type->shortname and shortname->type conversions. #30

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

nvivo
Copy link
Contributor

@nvivo nvivo commented Feb 3, 2017

@Horusiath, @Aaronontheweb Added some caching to type resolvers.

Related to #28 (comment)

I'm not able to run the performance tests here for some reason. Both VS and build runtests fails to run the performance tests. Would like to see some numbers at least before merging this. Please let me know if this works.

@@ -200,24 +203,27 @@ public static int GetTypeSize(this Type type)

public static string GetShortAssemblyQualifiedName(this Type type)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not requirement, but an idea: GetShortAssemblyQualifiedName is actually used in 2 external places (TypeSerializer and ObjectSerializer). In both places this string is converted to a byte array immediately after. What we could do here, is to make this method return a byte array directly, avoiding string conversions in the future.

Copy link
Contributor Author

@nvivo nvivo Feb 9, 2017

Choose a reason for hiding this comment

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

@Horusiath I was looking into this. There is an issue with this optimization. For ObjectSerializer this idea works fine, but TypeSerializer has it's own way to serialize values directy to the stream. I tried replacing the StringSerializer for the ByteArraySerializer and the tests didn't pass. We would need 2 caches one for each format, or change something else to make them equal.

Another thing is that since GetShortAssemblyQualifiedName is recursive, caching itself helps a little resolving names the first time. Then again, we could add second cache, but increase memory usage. Not sure if there would be significant gains.

Using the cache should make the protocol at least as fast as it was before. Worst case, a dictionary + lock could be faster in some scenarios.

This is getting complicated... =) I believe it's better to merge this as it is and try to optimize this later.

@Danthar
Copy link
Member

Danthar commented Feb 23, 2017

Is there anything that is preventing this PR from being merged ?

@nvivo
Copy link
Contributor Author

nvivo commented Mar 17, 2017

Should I update this to netstandard or is there another solution in the works?

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