Skip to content
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

refactor(tsc): do not store some typescript declaration file text in multiple places #17410

Merged

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jan 14, 2023

This stores some declaration files only in the tsc snapshot instead of in the snapshot and in the binary. Due to this, deno types now needs to get its information from a JS runtime.

Before: 69,987 KB (binary size on Windows)
After: 68,195 KB 69,324 KB (in this PR I previously moved everything to the snapshot and it had a greater reduction in size probably because the snapshot is compressed, but that led to an increase in memory usage of 0.5MB)

@dsherret dsherret changed the title refactor(tsc): store all typescript declaration files in typescript snapshot perf(size): do not store some typescript declaration file text in multiple places Jan 14, 2023
@dsherret dsherret changed the title perf(size): do not store some typescript declaration file text in multiple places refactor(tsc): do not store some typescript declaration file text in multiple places Jan 14, 2023
@@ -385,7 +385,7 @@ delete Object.prototype.__proto__;
// paths must be either relative or absolute. Since
// analysis in Rust operates on fully resolved URLs,
// it makes sense to use the same scheme here.
const ASSETS = "asset:https:///";
const ASSETS_URL_PREFIX = "asset:https:///";
Copy link
Member

Choose a reason for hiding this comment

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

We were supposed to change it to deno:https:///assets/, didn't we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet and I’m not sure yet. Need to investigate more

Comment on lines +110 to +119
let mut runtime = JsRuntime::new(RuntimeOptions {
startup_snapshot: Some(compiler_snapshot()),
extensions: vec![Extension::builder("deno_cli_tsc")
.ops(get_tsc_ops())
.build()],
..Default::default()
});
let global =
runtime.execute_script("get_assets.js", "globalThis.getAssets()")?;
let scope = &mut runtime.handle_scope();
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully deno types is not used by a hot path by any user code, this will incur about 20ms startup cost

Copy link
Member Author

@dsherret dsherret Jan 14, 2023

Choose a reason for hiding this comment

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

That sounds fine. People could cache it if they care about 20ms.

@dsherret dsherret merged commit 1712a88 into denoland:main Jan 14, 2023
@dsherret dsherret deleted the refactor_store_all_assets_in_tsc_snapshot branch January 14, 2023 14:36
bartlomieju pushed a commit that referenced this pull request Jan 16, 2023
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.

None yet

2 participants