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

Slimmer QueueUserWorkItemCallback #3157

Merged
merged 1 commit into from Feb 15, 2016

Conversation

Projects
None yet
6 participants
@benaadams
Collaborator

benaadams commented Feb 12, 2016

As per #3147 (comment)

No stored ExecutionContext when context is default

/cc @stephentoub something like this?

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub
Member

stephentoub commented Feb 12, 2016

cc: @ericeil

@ericeil

This comment has been minimized.

Show comment
Hide comment
@ericeil

ericeil Feb 12, 2016

Contributor

A couple of general thoughts on this:

  1. The work queues are exposed to debuggers, and I'm not sure how much they depend on the exact details of the workitem types. It's possible that this change will also require work in the debuggers to understand the new types. @noahfalk may know more.
  2. This change substantially increases the number of types that implement IThreadPoolWorkItem. This may impact the performance of the interface dispatch at the callsites for these methods, since they're now more polymorphic (requiring more interface dispatch cache entries). I don't know the current design of interface dispatch on CoreCLR, but there are typically performance "cliffs" as callsites become more and more polymorphic. This may not end up mattering, or be sufficiently offset by the GC perf gains from smaller (on average) work item objects, but it's something to watch out for.

Overall, though, I think this is a great change to try, with potential to help substantially in many scenarios.

Contributor

ericeil commented Feb 12, 2016

A couple of general thoughts on this:

  1. The work queues are exposed to debuggers, and I'm not sure how much they depend on the exact details of the workitem types. It's possible that this change will also require work in the debuggers to understand the new types. @noahfalk may know more.
  2. This change substantially increases the number of types that implement IThreadPoolWorkItem. This may impact the performance of the interface dispatch at the callsites for these methods, since they're now more polymorphic (requiring more interface dispatch cache entries). I don't know the current design of interface dispatch on CoreCLR, but there are typically performance "cliffs" as callsites become more and more polymorphic. This may not end up mattering, or be sufficiently offset by the GC perf gains from smaller (on average) work item objects, but it's something to watch out for.

Overall, though, I think this is a great change to try, with potential to help substantially in many scenarios.

@ericeil

This comment has been minimized.

Show comment
Hide comment
@ericeil

ericeil Feb 12, 2016

Contributor

cc: @kouvel

Contributor

ericeil commented Feb 12, 2016

cc: @kouvel

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Feb 12, 2016

Member

Thanks Eric! At the ICorDebug interfaces our interaction with the threadpool is identifying which threads are in the pool so no worries there. The SOS/DAC interfaces expose data from ThreadpoolMgr native class, but I am guessing this change wouldn't affect it. The relevant source is debug\daccess\request.cpp ClrDataAccess::GetThreadpoolData(struct DacpThreadpoolData *threadpoolData).

Higher up the stack its possible that Visual Studio might introspect on those types, but a cursory source search didn't find anything. I'll send an email to the VS debugger guys and ask them to take a peak.

Member

noahfalk commented Feb 12, 2016

Thanks Eric! At the ICorDebug interfaces our interaction with the threadpool is identifying which threads are in the pool so no worries there. The SOS/DAC interfaces expose data from ThreadpoolMgr native class, but I am guessing this change wouldn't affect it. The relevant source is debug\daccess\request.cpp ClrDataAccess::GetThreadpoolData(struct DacpThreadpoolData *threadpoolData).

Higher up the stack its possible that Visual Studio might introspect on those types, but a cursory source search didn't find anything. I'll send an email to the VS debugger guys and ask them to take a peak.

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Feb 12, 2016

Collaborator

This change substantially increases the number of types that implement IThreadPoolWorkItem...

Altered to follow @stephentoub's suggestion of having the null context use the same as non default context, and do the null check in execute. So there is just one new type for the default context.

Both null and non-default context now use the original type which is more or less the same other than removing the constructor that does the EC capture.

Collaborator

benaadams commented Feb 12, 2016

This change substantially increases the number of types that implement IThreadPoolWorkItem...

Altered to follow @stephentoub's suggestion of having the null context use the same as non default context, and do the null check in execute. So there is just one new type for the default context.

Both null and non-default context now use the original type which is more or less the same other than removing the constructor that does the EC capture.

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Feb 13, 2016

Member

Following up, my coworker over in VS debugger wrote me back and said he thinks this is fine from the VS perspective, they don't introspect into these types.

Member

noahfalk commented Feb 13, 2016

Following up, my coworker over in VS debugger wrote me back and said he thinks this is fine from the VS perspective, they don't introspect into these types.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Feb 13, 2016

Member

Thanks, @noahfalk, for following up.

Member

stephentoub commented Feb 13, 2016

Thanks, @noahfalk, for following up.

benaadams added a commit to benaadams/corert that referenced this pull request Feb 14, 2016

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Feb 14, 2016

Member

LGTM (once squashed).

Does this have the expected impact on your benchmark?

Member

stephentoub commented Feb 14, 2016

LGTM (once squashed).

Does this have the expected impact on your benchmark?

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Feb 14, 2016

Collaborator

Checking now

Collaborator

benaadams commented Feb 14, 2016

Checking now

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Feb 15, 2016

Collaborator

Its using the right object
QueueUserCallbackDefaultContext

Have to redo it as a release build to measure the allocations for same scenario correctly.

Collaborator

benaadams commented Feb 15, 2016

Its using the right object
QueueUserCallbackDefaultContext

Have to redo it as a release build to measure the allocations for same scenario correctly.

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Feb 15, 2016

Collaborator

Yep, behaves as suggested allocated bytes go from ~41MB to ~33MB

Before
Before-QueueUserWorkItemCallback

After
After-QueueUserWorkItemCallback

Collaborator

benaadams commented Feb 15, 2016

Yep, behaves as suggested allocated bytes go from ~41MB to ~33MB

Before
Before-QueueUserWorkItemCallback

After
After-QueueUserWorkItemCallback

@ericeil

This comment has been minimized.

Show comment
Hide comment
@ericeil

ericeil Feb 15, 2016

Contributor

Looks great. Thanks!

Contributor

ericeil commented Feb 15, 2016

Looks great. Thanks!

ericeil added a commit that referenced this pull request Feb 15, 2016

@ericeil ericeil merged commit 2ec6a40 into dotnet:master Feb 15, 2016

8 checks passed

CentOS7.1 x64 Checked Build Build finished. 804 tests run, 0 skipped, 0 failed.
Details
FreeBSD x64 Checked Build Build finished. 804 tests run, 0 skipped, 0 failed.
Details
OSX x64 Checked Build and Test Build finished. No test results found.
Details
Ubuntu x64 Checked Build and Test Build finished. No test results found.
Details
Windows_NT x64 Debug Build and Test Build finished. 5608 tests run, 0 skipped, 0 failed.
Details
Windows_NT x64 Release Build and Test Build finished. 5608 tests run, 0 skipped, 0 failed.
Details
Windows_NT x86 Debug Build Build finished. No test results found.
Details
Windows_NT x86 Release Build Build finished. No test results found.
Details

@benaadams benaadams deleted the benaadams:QueueUserWorkItemCallback branch Feb 15, 2016

@benaadams benaadams restored the benaadams:QueueUserWorkItemCallback branch Feb 15, 2016

benaadams added a commit to benaadams/corert that referenced this pull request Feb 16, 2016

@sajayantony

This comment has been minimized.

Show comment
Hide comment
@sajayantony

sajayantony Feb 20, 2016

Very cool 👍

sajayantony commented Feb 20, 2016

Very cool 👍

@sajayantony

This comment has been minimized.

Show comment
Hide comment
@sajayantony

sajayantony Feb 20, 2016

@geoffkizer we should get some updated numbers soon.

sajayantony commented Feb 20, 2016

@geoffkizer we should get some updated numbers soon.

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