Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Reduce uses of windows.d b/c it's slow (see PR #2336)#2400

Merged
thewilsonator merged 1 commit intodlang:masterfrom
n8sh:reduce-windows.d
Dec 18, 2018
Merged

Reduce uses of windows.d b/c it's slow (see PR #2336)#2400
thewilsonator merged 1 commit intodlang:masterfrom
n8sh:reduce-windows.d

Conversation

@n8sh
Copy link
Copy Markdown
Member

@n8sh n8sh commented Dec 9, 2018

In general avoided removing non-private core.sys.windows.windows imports from other core.sys.windows.* header files so imports-of-imports from them will not break. The exception is core.sys.windows.threadaux because it is imported by core.thread.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2400"

@n8sh
Copy link
Copy Markdown
Member Author

n8sh commented Dec 9, 2018

I can also do this in core.sys.windows.snmp and core.sys.windows.httpext because their imports of core.sys.windows.window are private.

@n8sh n8sh force-pushed the reduce-windows.d branch 2 times, most recently from dcb306a to 6de9e9d Compare December 9, 2018 14:56
@rainers
Copy link
Copy Markdown
Member

rainers commented Dec 9, 2018

In general avoided removing non-private core.sys.windows.windows imports from other core.sys.windows.* header files so imports-of-imports from them will not break.

import is private by default, so only "public imports" might be affected.

Selective imports do create public symbols, though (albeit deprecated). So this could be a breaking change causing ambiguities before the deprecated behaviour is removed from the compiler. It might be less invasive to not list imported symbols (while mostly desirable in general some lists look a bit excessive).

Do you have some example numbers what the compile time improvements are?

@n8sh
Copy link
Copy Markdown
Member Author

n8sh commented Dec 9, 2018

Selective imports do create public symbols, though (albeit deprecated). So this could be a breaking change causing ambiguities before the deprecated behaviour is removed from the compiler.

Didn't know that, thanks. This PR should not be merged before I fix this.

Do you have some example numbers what the compile time improvements are?

I assumed there would only be a noticeable speed difference with uncached reads from a non-SSD drive (I'm assuming the main speed penalty from importing windows.d is opening ~32 other files), but benchmarking would still be a good idea.

@thewilsonator
Copy link
Copy Markdown
Contributor

Ping @n8sh

@n8sh
Copy link
Copy Markdown
Member Author

n8sh commented Dec 17, 2018

@thewilsonator removed top-level selective imports as @rainers recommended. I also did an informal benchmark with a non-optimized DMD development build. Compiling the following program went from 640 milliseconds (best of 40 tries) to 240 milliseconds (best of 40 tries):

import core.internal.abort;
import core.runtime;
import core.sync.condition;
import core.sync.mutex;
import core.sync.semaphore;
import core.sys.windows.httpext;
import core.sys.windows.snmp;
import core.sys.windows.threadaux;
import core.thread;
import core.time;

void main(string[] args) {}

@n8sh
Copy link
Copy Markdown
Member Author

n8sh commented Dec 17, 2018

That was a more noticeable difference than I expected. I'll remove the remaining imports of windows.d from other core.sys.windows.* files.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants