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

Reduce XName lookups #19

Closed
jods4 opened this issue Oct 26, 2020 · 6 comments · Fixed by #23
Closed

Reduce XName lookups #19

jods4 opened this issue Oct 26, 2020 · 6 comments · Fixed by #23
Labels
enhancement New feature or request

Comments

@jods4
Copy link
Contributor

jods4 commented Oct 26, 2020

Looking at the generated code, I'm a bit surprised that getters systematically use XName.Get().
When parsing files with hundreds if not thousands of repeated tags and attributes, it's a lot of unrequired calls, which are not free as they involve several thread-safe hashtables of weakreferences underneath.

I think it could be a fairly trivial code fix: simply introduce static variable for each element and attribute name, and use that instead of the XName.Get() expression in the code.

Impact analysis of the change

One could be concerned about overall memory usage (when parsing), leaks (XName uses WeakRef and releases namespaces and names once they're not referenced anymore), cost for elements that are not accessed (e.g. large xsd with only a tiny subset accessed at runtime), and initialization time.

For elements, there is no impact at all.
The static ctor already creates every XName and saves it in localElementDictionary and sometimes in contentModel as well.
So the work to instantiate the XNames is already done anyway and they will be kept alive forever.

Attributes are created lazily on first access. They are kept alive forever though, as only full namespaces are collected when unused, not individual names.
Given all the work done anyway on elements, I don't think adding the attributes is a significant change. It should be an all-or-nothing design.
If that's really a concern, it could be done on elements only.

So in summary this change would:

  • keep static initialization time constant.
  • keep overall memory usage the same.
  • reduce code/IL size.
  • speed-up runtime access.
@mamift
Copy link
Owner

mamift commented Oct 27, 2020

Seems like a sensible suggestion, and with this reply I note my approval for either yourself or anyone else to begin work on a PR or issue one in the future.

@mamift mamift added the enhancement New feature or request label Nov 2, 2020
@jods4 jods4 mentioned this issue Feb 3, 2021
@jods4
Copy link
Contributor Author

jods4 commented Feb 3, 2021

I made a PR to improve this :)

You have inherited a pretty old codebase... It would benefit from some upgrades but that's a daunting task :/
So many thanks for keeping this alive, because this project is very useful.

@mamift mamift closed this as completed in #23 Feb 4, 2021
@jods4
Copy link
Contributor Author

jods4 commented Feb 4, 2021

@mamift That PR had a stupid mistake where the backing field is typed as string, should have been XName of course.
#25 fixes that.

@jods4
Copy link
Contributor Author

jods4 commented Feb 14, 2021

@mamift I found a bug in this PR today.
When your XSD contains inheritance, the child class need access to the parent XName fields (for its BuildElementDictionary).
So right now the generated code may not compile.
I fixed this by making the XName fields internal instead of private and added EditorBrowsable.Never so that they don't show up as static members for users.
PR coming soon (w/ the nullable refs).

@jods4 jods4 mentioned this issue Feb 14, 2021
@jods4
Copy link
Contributor Author

jods4 commented Feb 17, 2021

@mamift Small bit of feedback here.
Using this in our real work, I noticed that EditorBrowsableState.Never is not respected by VS 2019.
I was quite surprised and I learnt that VS doesn't apply this attribute in the same solution.

It kind of makes sense as if you develop a lib you probably want to see all members in IDE (and hide them from consumers of the lib).
That perspective is of course a bit broken when it comes to generated code, which is a bit of a lib in your own code...

So just so you know, this attribute is useless right now but there is a long standing Roslyn issue about it:
dotnet/roslyn#4434 (comment)

@jods4
Copy link
Contributor Author

jods4 commented Feb 17, 2021

Well, someone pointed this isn't actually the issue here and the behavior was expected.
I opened dotnet/roslyn#51269 to see if that behaviour could be changed.
If not, you can remove the EditorBrowsable from codegen as they're useless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants