Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Faster common header handling#554

Closed
benaadams wants to merge 1 commit into
aspnet:devfrom
benaadams:known-header-offsets
Closed

Faster common header handling#554
benaadams wants to merge 1 commit into
aspnet:devfrom
benaadams:known-header-offsets

Conversation

@benaadams
Copy link
Copy Markdown
Contributor

No description provided.

Also has "Faster output header handling" aspnet#528 in it :-/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Spelling. Fix commit message too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This word seems to haunt me for some reason

@benaadams benaadams force-pushed the known-header-offsets branch from 56ca33f to d6463c7 Compare January 5, 2016 23:25
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I chatted w/ @jkotas, and he suggested this could be simplified to:

static bool initializer;

public static void InitializeJitConstants()
{
     initializer = BitConverter.IsLittleEndian;
}

All static fields of a class get initialized at once.

He also suggested it's better to avoid static variables altogether. This could probably be achieved by making Constants non-static and referencing the singleton instance using a property in ServiceContext. I think we should probably do this instead, so we don't need to worry about static initialization order.

@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 5, 2016

Are the two commits in this PR related? Does one require the other?

@benaadams
Copy link
Copy Markdown
Contributor Author

Yea, was just looking at that - I think it was because it was related to the earlier commit that was in here; will split again.

@benaadams benaadams force-pushed the known-header-offsets branch from d6463c7 to f096964 Compare January 5, 2016 23:51
@benaadams
Copy link
Copy Markdown
Contributor Author

Removed other PR

@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 6, 2016

Given the extra complexity this adds for the relatively small performance improvement, I'm not very inclined to merge this PR.

@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 6, 2016

This is really cool, but a little much for a 1% perf gain.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants