-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remove wasteful virtuals according to SizeBench #11889
Conversation
@miniksa Some of these removals might be bad. Can you check which one's I need to revert? For instance this pulls in interactivity/win32 code into interactivity/base and that's kinda whack. On the other hand some of these changes actually make sense once you check neighboring code. For example, So... yeah. Just let me know what to revert and I'll do that. No hard feelings. The |
2f0ff80
to
f42c8fc
Compare
This comment has been minimized.
This comment has been minimized.
07c82d3
to
e352a45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I have hidden pending comments
EDIT: no it lost them somewhere damn it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
65/65. I think I called out the important bits.
e352a45
to
185064e
Compare
This comment has been minimized.
This comment has been minimized.
185064e
to
05ff1c0
Compare
05ff1c0
to
cfab0f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this. I really liked how we used to have all these neat little silos that didn't know about the impl details of other parts of the codebase. But, I get that it's bad for performance, and we never really leveraged that siloing, so 🤷
@@ -31,7 +31,7 @@ namespace Microsoft::Console::Render | |||
std::optional<CursorOptions> cursorInfo; | |||
}; | |||
|
|||
class IRenderEngine | |||
class __declspec(novtable) IRenderEngine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have been doing this just always? I literally had no idea this existed, but seems really valuable for interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we probably should ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait I'm dismissing my own review since this still doesn't build yet, wanna make sure the headers don't get included in a totally bonkers fashion
I fixed the compilation error btw. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could have the value of an interface (SPECIFICALLY: the restriction on what somebody holding a pointer can do. I don't care about swapping the impementation) without the cost.
@@ -31,7 +31,7 @@ namespace Microsoft::Console::Render | |||
std::optional<CursorOptions> cursorInfo; | |||
}; | |||
|
|||
class IRenderEngine | |||
class __declspec(novtable) IRenderEngine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we probably should ;P
src/renderer/vt/vtrenderer.hpp
Outdated
@@ -15,13 +15,13 @@ Author(s): | |||
|
|||
#pragma once | |||
|
|||
#include <../host/VtIo.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you've explained that these don't matter to this specific compiler, but I do think we should follow the prevailing style!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is: ""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is even worse! This file is src/renderer/vt/vtrenderer.hpp
and I'm trying to include src/host/VtIo.hpp
.
With "" this would then be a MS specific search path, because MSVC is the only compiler which allows "" includes to search in the current directories of previously included files. I've replaced it with "../../host/VtIo.hpp"
.
@@ -31,9 +31,14 @@ namespace TerminalCoreUnitTests | |||
}; | |||
#endif | |||
|
|||
namespace Microsoft::Console::VirtualTerminal | |||
{ | |||
class VtIo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You included the header and forward-declared VtIo?
Given that we are not making as extensive of changes as we were when you wrote this, are we still saving a whole 10kb? |
Honestly, I feel like we can still do that. I just personally feel like we don't necessarily need to keep interfaces if no one has been using them for years. This makes it very unlikely anyone will make use of them in the next years either. If we need to turn any of these structs into an interface one day, it's likely easier to "interfacify" code than remove those interfaces, since at the point you "interfacify" them you already have the knowledge about the requirements of at least 2 implementations and not just the one for which it was originally written. I'll fix the issues in a minute and go over the PR again. |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
This commit removes some pure virtual base classes from conhost,
found with the help of SizeBench. This reduces binary size by 5kB.
The reduction in code size however is the main benefit of this.
Additionally this fixes a mysterious, undebuggable crash in
~RenderThread(), caused by a Control Flow Guard failure when
the class was destroyed over its IRenderThread interface.
PR Checklist
Validation Steps Performed