Skip to content

Conversation

@gmittert
Copy link
Contributor

@gmittert gmittert commented Jun 3, 2019

Since Dispatch threads have a 64k stack size, tests were failing due to
blowing the stack on Windows. This runs the tests in thread with a
larger stack size. This fixes some 90ish sourcekit tests on Windows.

@gmittert gmittert requested a review from benlangmuir June 3, 2019 20:54
@gmittert
Copy link
Contributor Author

gmittert commented Jun 3, 2019

@compnerd

@compnerd
Copy link
Member

compnerd commented Jun 3, 2019

CC: @akyrtzi @benlangmuir

@compnerd
Copy link
Member

compnerd commented Jun 3, 2019

@swift-ci please test

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate, it's going to be creating a new pthread every time to query the AST. Is there no way to make it work without making this change ?
I find strange that dispatch threads have limited stack also on Darwin but we did not have issues with querying the AST.

If there is no way around it I'd at least suggest to only have this change in effect when compiling for Windows.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/apple/swift-corelibs-libdispatch/blob/6c5b3ba86fce0480f702ba13fcb5f656af090b20/src/queue.c#L5175

This is hardcoded into libdispatch. There is no way to adjust the stack size. It is very likely that you are skimming past pretty close to the limit.

Copy link
Contributor Author

@gmittert gmittert Jun 3, 2019

Choose a reason for hiding this comment

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

That's my current theory. A sample count of frame sizes seems to vouch to that. But why Windows uses slightly more memory, I'm not sure yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

That stack trace looks to be from syntactic parsing, it is unrelated to ASTUnit::Implementation::consumeAsync.
Is the change inside consumeAsync really necessary or are the other changes making it redundant ?

Copy link
Contributor Author

@gmittert gmittert Jun 4, 2019

Choose a reason for hiding this comment

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

The consume async change is needed here. "S:\b\swift\bin\sourcekitd-test.EXE" "-req=cursor" "-pos=9:25" "S:\swift\test\SourceKit\CursorInfo\rdar_30248264.swift" "--" "S:\swift\test\SourceKit\CursorInfo\rdar_30248264.swift" stack overflows without it on windows. I'm currently trying to find a better way to make the change though. At worst, I'll #if defined(_WIN32) it, but I'd rather do something nicer.

The stack trace was more an example from another test that was stack overflowing showing that Windows typically does currently use more stack memory than Darwin. It was relatively representative of what I've seen in other areas.

@swift-ci
Copy link
Contributor

swift-ci commented Jun 3, 2019

Build failed
Swift Test OS X Platform
Git Sha - d1122a6fa0f7e311848fcca965933005668734f7

Since Dispatch threads have a 64k stack size, tests were failing due to
blowing the stack on Windows. This runs the tests in thread with a
larger stack size. This fixes some 90ish sourcekit tests on Windows.
@gmittert gmittert force-pushed the 64KOughtToBeEnoughForAnybody branch from d1122a6 to fa513db Compare June 4, 2019 22:33
@gmittert
Copy link
Contributor Author

gmittert commented Jun 4, 2019

Bleh, no idea what's causing the difference in stack sizes. I sadly just put it behind an if defined WIN32.

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Jun 4, 2019

Build failed
Swift Test Linux Platform
Git Sha - d1122a6fa0f7e311848fcca965933005668734f7

@gmittert gmittert merged commit 2966f33 into swiftlang:master Jun 5, 2019
@gmittert gmittert deleted the 64KOughtToBeEnoughForAnybody branch October 15, 2019 21:06
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.

4 participants