Skip to content

Add a WebAssembly version of System.Console. - #35555

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
vargaz:wasm-console
Apr 29, 2020
Merged

Add a WebAssembly version of System.Console.#35555
stephentoub merged 1 commit into
dotnet:masterfrom
vargaz:wasm-console

Conversation

@vargaz

@vargaz vargaz commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@vargaz

vargaz commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

This has about half the size (linked) as the normal unix version (21k vs 45k).

@stephentoub

Copy link
Copy Markdown
Member

This has about half the size (linked) as the normal unix version (21k vs 45k).

What accounts for the difference?

@vargaz

vargaz commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

All the not implemented methods plus the simplified error handling, i.e. Interop.GetExceptionForIoErrno () is not used.

@stephentoub

Copy link
Copy Markdown
Member

All the not implemented methods

What keeps them linked in? Presumably the app isn't calling them.

@vargaz

vargaz commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

There is a huge amount of code which reads the terminal database. Some virtual methods like the Read() methods are also retained.

@stephentoub

Copy link
Copy Markdown
Member

There is a huge amount of code which reads the terminal database. Some virtual methods like the Read() methods are also retained.

Ok, thanks. I'm fine with this being merged, though I think it'd be good in the future for us to try to factor things in such a way that the linker could get rid of more, e.g. the terminal database, if it's not needed. It shouldn't be needed if the only thing the app is using is Console.ReadLine, for example. And it'd be nice if the size gains could accrue in the future to apps on other Unixes as well.

cc: @eerhardt

@danmoseley danmoseley added arch-wasm WebAssembly architecture area-System.Console labels Apr 28, 2020
@ghost

ghost commented Apr 28, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

@eerhardt

Copy link
Copy Markdown
Member

though I think it'd be good in the future for us to try to factor things in such a way that the linker could get rid of more

My understanding is the issue is the virtual-ness of Stream. Since ConsoleStream derives from Stream, and the linker needs to preserve the base Stream.Read (for obvious reasons), I don't think it will trim ConsoleStream.Read. It would probably be pretty hard to figure out statically if a ConsoleStream is ever passed into a method that takes a Stream and calls .Read on it.

cc @vitek-karas @MichalStrehovsky @sbomer

@vargaz

vargaz commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

The terminal db code is being kept alive by:
ConsolePal:Write() -> UpdatedCachedCursorPosition() -> GetWindowSize() -> TerminalFormatStrings.Instance.

@stephentoub

Copy link
Copy Markdown
Member

My understanding is the issue is the virtual-ness of Stream.

Isn't that Read small potatoes compared to the terminal db?

@eerhardt

Copy link
Copy Markdown
Member

Isn't that Read small potatoes compared to the terminal db?

My point wasn't specifically Read, it was that the virtual methods on a derived Stream aren't able to be trimmed.

@stephentoub

stephentoub commented Apr 28, 2020

Copy link
Copy Markdown
Member

My point wasn't specifically Read, it was that the virtual methods on a derived Stream aren't able to be trimmed.

Understood, thanks. I'm highlighting that most of those overrides throw just like the one in this PR, they just throw a more appropriate exception that requires a tiny bit more logic to compute. So it doesn't look like we're saving a ton by having a custom ConsoleStream just for wasm; correct me if I'm wrong. The larger piece here seems to be the avoidance of the terminal db code, which ended up getting brought in per #35555 (comment) as part of 2ef9d36

@stephentoub
stephentoub merged commit adcdef8 into dotnet:master Apr 29, 2020
@vargaz
vargaz deleted the wasm-console branch April 29, 2020 07:15
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasm WebAssembly architecture area-System.Console

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants