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

Implement safe fiber recycling, fixes #753 #754

Merged
merged 2 commits into from Jul 30, 2014

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Jul 30, 2014

I couldn't find a zerofill algorithm in D so I had to improvise one. This is rarely even important to do b/c D automatically initializes variables as zero-filled, though TaskLocal variables are already initialized when the fiber comes around again.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2014

Member

Hm.. the m_flsInit array is specifically meant to avoid this kind of cost for every new task. Since the access is always guarded by a check, there shouldn't a safety issue. What's still missing though is to destroy all variables after each task has ended. This is also very important when reference counting or similar things are going on.

Member

s-ludwig commented Jul 30, 2014

Hm.. the m_flsInit array is specifically meant to avoid this kind of cost for every new task. Since the access is always guarded by a check, there shouldn't a safety issue. What's still missing though is to destroy all variables after each task has ended. This is also very important when reference counting or similar things are going on.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 30, 2014

Contributor

What's still missing though is to destroy all variables after each task has ended.

Sounds fair, I thought maybe a delegate in the core fiber would do to catch the proper type info there and destroy correctly?

Contributor

etcimon commented Jul 30, 2014

What's still missing though is to destroy all variables after each task has ended.

Sounds fair, I thought maybe a delegate in the core fiber would do to catch the proper type info there and destroy correctly?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 30, 2014

Contributor

I remember reading that the delegate would make a GC allocation, so I replaced it by a function reference and a void*. Looks lightweight enough to me and certainly more stable than recycling the data on new connections .. which on my application meant inheriting someone else's account if you didn't have a session id.. xD

Contributor

etcimon commented Jul 30, 2014

I remember reading that the delegate would make a GC allocation, so I replaced it by a function reference and a void*. Looks lightweight enough to me and certainly more stable than recycling the data on new connections .. which on my application meant inheriting someone else's account if you didn't have a session id.. xD

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2014

Member

Looks about right, only

  • Instead of TaskDestructor, it should instead be a struct FLSInfo, containing the destructor function pointer, as well as the offset of the FLS variable instead of the pointer, so that it stays the same for all tasks
  • TaskDestructor[] m_destructors; should be static FLSInfo[] ms_flsInfo; - no need to create one array per task anymore when the offset is stored
  • Only fields with m_flsInit[id] == true should be destroyed
  • Only fields with std.traits.hasElaborateDestructor!T should be destroyed for efficiency (don't set the destructor function pointer for other types) - maybe types with hasAliasing!Tcould instead be zeroed out to avoid false GC pointers
Member

s-ludwig commented Jul 30, 2014

Looks about right, only

  • Instead of TaskDestructor, it should instead be a struct FLSInfo, containing the destructor function pointer, as well as the offset of the FLS variable instead of the pointer, so that it stays the same for all tasks
  • TaskDestructor[] m_destructors; should be static FLSInfo[] ms_flsInfo; - no need to create one array per task anymore when the offset is stored
  • Only fields with m_flsInit[id] == true should be destroyed
  • Only fields with std.traits.hasElaborateDestructor!T should be destroyed for efficiency (don't set the destructor function pointer for other types) - maybe types with hasAliasing!Tcould instead be zeroed out to avoid false GC pointers
Zerofill m_fill for safe fiber recycling, fixes #753
Reset initialization vector also

Added destructor support

Changed destructor to function and pointer

do not expose TaskDestructor

Using static struct for destructors

Checking for empty FLSInit
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 30, 2014

Contributor

Alright, all good and tested on a web app. Not sure on the long run how it's going to fare though but it looks alright

Contributor

etcimon commented Jul 30, 2014

Alright, all good and tested on a web app. Not sure on the long run how it's going to fare though but it looks alright

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2014

Member

Apart from the destructor, looks good to merge.

Member

s-ludwig commented Jul 30, 2014

Apart from the destructor, looks good to merge.

Additional precautions for possible access violations
Remove unsafe TaskLocal destructor

Remove bugs
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 30, 2014

Contributor

Done

Contributor

etcimon commented Jul 30, 2014

Done

@etcimon etcimon changed the title from Zerofill m_fill for safe fiber recycling, fixes #753 to Implement safe fiber recycling, fixes #753 Jul 30, 2014

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 30, 2014

Member

Okay, thanks! Merging in.

Member

s-ludwig commented Jul 30, 2014

Okay, thanks! Merging in.

s-ludwig added a commit that referenced this pull request Jul 30, 2014

Merge pull request #754 from etcimon/tls-safety
Implement safe fiber recycling, fixes #753

@s-ludwig s-ludwig merged commit fafd280 into vibe-d:master Jul 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@etcimon etcimon deleted the etcimon:tls-safety branch Jul 30, 2014

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