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

Implement safe fiber recycling, fixes #753 #754

Merged
merged 2 commits into from
Jul 30, 2014
Merged

Implement safe fiber recycling, fixes #753 #754

merged 2 commits into from
Jul 30, 2014

Conversation

etcimon
Copy link
Contributor

@etcimon 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
Copy link
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.

@etcimon
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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

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
Copy link
Contributor Author

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

fiber.ms_flsInfo[m_id].destroy(fiber.m_fls);
fiber.m_flsInit[m_id] = false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is usually going to crash. A TaskLocal will only be destroyed when the thread has ended. However, in that case the event loop has already been quit and CoreTask.getThis() returns null. So I'd just remove this.

@s-ludwig
Copy link
Member

Apart from the destructor, looks good to merge.

Remove unsafe TaskLocal destructor

Remove bugs
@etcimon
Copy link
Contributor Author

etcimon commented Jul 30, 2014

Done

@etcimon etcimon changed the title Zerofill m_fill for safe fiber recycling, fixes #753 Implement safe fiber recycling, fixes #753 Jul 30, 2014
@s-ludwig
Copy link
Member

Okay, thanks! Merging in.

s-ludwig added a commit that referenced this pull request Jul 30, 2014
Implement safe fiber recycling, fixes #753
@s-ludwig s-ludwig merged commit fafd280 into vibe-d:master Jul 30, 2014
@etcimon etcimon deleted the tls-safety branch July 30, 2014 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants