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

DateTime.ToString(“o”) allocates 32 objects, #7836

Merged
merged 8 commits into from Oct 29, 2016

Conversation

@Petermarcu
Member

Petermarcu commented Oct 26, 2016

Fix for #7815

I'm still working on running the corefx tests on this change and possibly adding some but wanted to start the CR process.

I'm seeing an ~90% reduction in allocation count and ~80% reduction in allocated bytes after this change.

@tarekgh @weshaggard @stephentoub

@tarekgh

This comment has been minimized.

Show comment
Hide comment
@tarekgh

tarekgh Oct 26, 2016

Member

@Petermarcu other than my minor comments, LGTM.

I'm seeing an ~90% reduction in allocation count and ~80% reduction in allocated bytes after this change.

just curious how did you measure it?

Member

tarekgh commented Oct 26, 2016

@Petermarcu other than my minor comments, LGTM.

I'm seeing an ~90% reduction in allocation count and ~80% reduction in allocated bytes after this change.

just curious how did you measure it?

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 27, 2016

Member

@dotnet-bot test Linux ARM Emulator Cross Debug Build please

Member

Petermarcu commented Oct 27, 2016

@dotnet-bot test Linux ARM Emulator Cross Debug Build please

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 27, 2016

Member

@dotnet-bot test Linux ARM Emulator Cross Release Build please

Member

Petermarcu commented Oct 27, 2016

@dotnet-bot test Linux ARM Emulator Cross Release Build please

Petermarcu added some commits Oct 27, 2016

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 28, 2016

Member

@tarekgh , Can you take one last look? I updated it to handle the DateTimeOffset failure I was seeing and to leverage an existing method I found while investigating. All other feedback was incorporated.

@redknightlois, I didn't see a noticable difference with and without the attribute. If you want to try and measure the overhead and see if there is something actionable for the JIT team, that would be awesome! For now, I'm going to trust the JIT on this one :)

Member

Petermarcu commented Oct 28, 2016

@tarekgh , Can you take one last look? I updated it to handle the DateTimeOffset failure I was seeing and to leverage an existing method I found while investigating. All other feedback was incorporated.

@redknightlois, I didn't see a noticable difference with and without the attribute. If you want to try and measure the overhead and see if there is something actionable for the JIT team, that would be awesome! For now, I'm going to trust the JIT on this one :)

builder[builder.Length - index] = (char)('0' + (val % 10));
val = val / 10;
index++;
}

This comment has been minimized.

@tarekgh

tarekgh Oct 28, 2016

Member

I am seeing you have removed the second loop. I would suggest we just assert val == 0 if we need this method be reliable in case someone else later use it wrong way

@tarekgh

tarekgh Oct 28, 2016

Member

I am seeing you have removed the second loop. I would suggest we just assert val == 0 if we need this method be reliable in case someone else later use it wrong way

@tarekgh

This comment has been minimized.

Show comment
Hide comment
@tarekgh

tarekgh Oct 28, 2016

Member

I put another minor comment, other than that LGTM

Member

tarekgh commented Oct 28, 2016

I put another minor comment, other than that LGTM

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 28, 2016

Member

Thanks. I've addressed all feedback. I'll commit once green.

Member

Petermarcu commented Oct 28, 2016

Thanks. I've addressed all feedback. I'll commit once green.

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 28, 2016

Member

@dotnet-bot test Windows_NT x64 Release please

Member

Petermarcu commented Oct 28, 2016

@dotnet-bot test Windows_NT x64 Release please

@Petermarcu

This comment has been minimized.

Show comment
Hide comment
@Petermarcu

Petermarcu Oct 28, 2016

Member

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

Member

Petermarcu commented Oct 28, 2016

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

@Petermarcu Petermarcu merged commit 026beb9 into dotnet:master Oct 29, 2016

14 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 legacy_backend Checked Build and Test Build finished.
Details
Windows_NT x86 ryujit Checked Build and Test Build finished.
Details

@Petermarcu Petermarcu deleted the Petermarcu:DateTime branch Oct 29, 2016

@weshaggard

This comment has been minimized.

Show comment
Hide comment
@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 30, 2016

Member

Opened dotnet/corert#2109 so that we do not forget to port it.

Member

jkotas commented Oct 30, 2016

Opened dotnet/corert#2109 so that we do not forget to port it.

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

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