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

Design Meeting Notes, 6/18/2024 #58919

Open
DanielRosenwasser opened this issue Jun 18, 2024 · 2 comments
Open

Design Meeting Notes, 6/18/2024 #58919

DanielRosenwasser opened this issue Jun 18, 2024 · 2 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Coverage Reporting

#58850

  • C8
    • Coverage system that wraps V8, the JS engine Node uses.
    • We currently have a task like this where you can run npm tests -- --no-lint --coverage.
    • Problem: when you bundle our entire codebase with esbuild, the file is huge. The built-in tools for C8 don't work well for processing a coverage report on TypeScript.
  • Monocart: a different preview tool that not only can preview the entire produced file, but can work with sourcemaps and give details on branches that are or are not hit.
  • Also has an output mode for codecov, a service to track code testing coverage.
  • Probably don't want to have the comment that says "your coverage has fallen!" and gates merging, but we do want a coverage report.
    • Downside: it makes CI run slower (doubles it!).
  • Would be great to figure out "which tests rely on a given branch executing?"
  • Why are we doing this if we don't want to actually make it visible?
    • Good question.
    • We have >97% coverage of the checker, but many many uncovered branches.
  • We want to wait on ways to make this all run faster before it's in CI - but first let's add an optional peer dep to c8 so people can run it locally.

Knip

#56817

  • Utility to flag down functions not used outside the file, drop unused files, and drop utilities that are not used but also not exported as part of the public API.
  • Make exceptions by adding a /** @knipignore */ tag to the preceding comment.
  • Last time we talked about this, wasn't there an installation time delay?
    • Yes, but since then knip has shaved down its dependency tree.
  • What about running time?
    • About 7s - not bad.
    • We're thinking of just doing it in CI.

@internal Tagging

  • Basically a way to formally encode what --stripInternal is supposed to do, and say it only applies to JSDoc comments.
    • Allows you to write // Do not add an @internal tag to the next declaration, since TypeScript will now consider that unrelated.
  • With --stripInternal, we've always said "it has bugs, ehhh, arguable behavior."
  • This is a way to make it more formal - or at least more predictable.
  • Does this add overhead because we'd now start to ask for JSDoc comments in emit.

More "Primitive" Operations in Core Utilities

#58873

  • Started out with a trace where the internal some function was just a big hot path.
  • Lots of operations that implicitly do more work than you might expect - but have decent alternatives.
    • || checks for truthiness, but ?? only checks for null/undefined.
    • Explicit checks for undefined are faster than truthiness.
    • for-of loops create iterators but for (let i = 0; i < arr.length; i++) doesn't.
  • PR makes some changes, mostly isolated to core.ts to avoid too much noise.
  • Unfortunate that there's no free lunch here. The abstraction comes at an explicit cost.
@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Jun 18, 2024
@fatcerberus
Copy link

for-of loops create iterators but for (let i = 0; i < arr.length; i++) doesn't.

I'd have expected for (const foo of foos) {} to be highly optimized by engines at this point. Since the iterator for arrays doesn't actually do anything, I'd be surprised if most engines didn't optimize it away in this case.

@Jamesernator
Copy link

I'd have expected for (const foo of foos) {} to be highly optimized by engines at this point. Since the iterator for arrays doesn't actually do anything, I'd be surprised if most engines didn't optimize it away in this case.

Still not really, a for-of-loop is still often many times slower than a for-loop, e.g. this has a difference of 10x in v8 , and 4x in jsc (curiously inlining the forOfLoop function makes v8 have a 4x difference as well):

const arr = Array.from({ length: 10_000_000 }, () => Math.random());

function forLoop() {
    let sum = 0;
    for (let i = 0; i < arr.length; i += 1) {
        sum += arr[i];
    }
}

function forOfLoop() {
    let sum = 0;
    for (const v of arr) {
        sum += v;
    }
}

console.time("for-loop");
for (let i = 0; i < 100; i += 1) {
    forLoop();
}
console.timeEnd("for-loop");

console.time("for-of-loop");
for (let i = 0; i < 100; i += 1) {
    forOfLoop();
}
console.timeEnd("for-of-loop");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

3 participants