Use dynamic modules for closures instead of -CaptureContext

microsoft / DbgShell

A PowerShell front-end for the Windows debugger engine.

MIT License

674 stars 89 forks source link

Use dynamic modules for closures instead of -CaptureContext #47

Closed Zhentar closed 5 years ago

Zhentar commented 5 years ago

This branch replaces every use of -CaptureContext with dynamic modules, via either NewBoundScriptBlock (when functions needed to be captured) or GetNewClosure() (when only variables needed to be captured.

I pursued this primarily as a startup time optimization (in my measurements with a Release build, I saw a ~1 second improvement in startup CPU time, ~1.5 second improvement in wall time; running from source with a debugger hooked sees a much larger benefit). However, it does also seem like a much more maintainable option than Get-PSContext as well

msftclas commented 5 years ago

All CLA requirements met.

Zhentar commented 5 years ago

A couple notes I forgot to add:

I don't have particularly high confidence in the quality or thoroughness of my testing, since I wasn't sure how to test a number of the pieces.
I didn't update the PreserveHeaderContext case yet because I only noticed it this morning.
I didn't take out any of the existing CaptureContext handling (which means there are still a number of defensive SetContextStartMarker calls that could be removed for further startup time improvement!). Happy to add it on to the pull request if you'd like :smiley:

jazzdelightsme commented 5 years ago

Thank you for sending this! Sorry for being slow to respond; life got busy and then I had some hardware failure on top of that. I will try to get some time to review this over the next week or two.

Zhentar commented 5 years ago

No worries, you still responded in under 24 hours so that definitely doesn't count as slow 😄

After much gnashing of teeth, I managed to come up with something that I think is semantically equivalent to -PreserveHeaderContext with the Get-PsContext approach: https://github.com/Zhentar/DbgShell/commit/608a98f80a32b6a22983b452d7bed6e1b0e4b4bd Unfortunately it requires reflection to call an internal method on ScriptBlock. I presume that will be a no-go*, so I won't update the pull request with it. (Oh, the pull request updates automatically. I'll just revert the commit tomorrow then)

* I think it's fine, but I'm also totally comfortable with detouring code by overwriting jitted code in memory so maybe you shouldn't entirely trust me

jazzdelightsme commented 5 years ago

NewBoundScriptBlock, where've you been all my life?! 😍

(I see it's been around a long time; but if I ever knew about it, I'd forgotten--what a handy little method!)

I presume [internal reflection] will be a no-go

Well... of course it would be better if we could do it without reaching into internals... but I don't particularly like the existing solution, either.

What was the purpose of calling into DoInvokeReturnAsIs? Is it just so that dollar underbar ($_) is set, or is there more to it?

Zhentar commented 5 years ago

Oh! Forgot to explain over here too; posted a bit of a description in https://github.com/PowerShell/PowerShell/issues/8208 Getting the script block to execute in the module scope and have underbar populated is difficult at best... $input and/or $args (or as bound parameters) are easily doable, though.

jazzdelightsme commented 5 years ago

Thanks for a great contribution!