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

Update ReadonlyArray.ts #1930

Closed
wants to merge 3 commits into from
Closed

Conversation

christiantakle
Copy link
Contributor

@christiantakle christiantakle commented Mar 25, 2024

Fix: #1929

Not sure about adding a test here since the size of the input depend on nodejs and/or the machine its running on.

Example using the change

Note the source code is inline so its simpler to play with.

/** Inlined from source code */
const isEmpty = <A>(as: ReadonlyArray<A>): as is readonly [] => as.length === 0;
// const empty: ReadonlyArray<never> = RNEA.empty
const empty: ReadonlyArray<never> = [];

const value = {};
// const a: Array<Array<typeof value>> = [new Array(112201).fill(value)];
/**
 * 112201, the number that failed in production and on an Apple M1 Max 64gb.
 * I had to increase the number to see a similar failure on my desktop workstation hence the
 * `* 2`
 */
const ass: Array<Array<typeof value>> = [new Array(112201 * 2).fill(value)];

// chainWithIndex from ReadonlyArray
const chainWithIndex =
  <A, B>(f: (i: number, a: A) => ReadonlyArray<B>) =>
  (as: ReadonlyArray<A>): ReadonlyArray<B> => {
    if (isEmpty(as)) {
      return empty;
    }
    const out: Array<B> = [];
    for (let i = 0; i < as.length; i++) {
      out.push(...f(i, as[i]));
    }
    return out;
  };

// Alternative implementation
export const chainWithIndex2 =
  <A, B>(f: (i: number, a: A) => ReadonlyArray<B>) =>
  (as: ReadonlyArray<A>): ReadonlyArray<B> => {
    if (isEmpty(as)) {
      return [];
    }
    const out: Array<B> = [];
    for (let i = 0; i < as.length; i++) {
      const bs = f(i, as[i]);
      for (let j = 0; j < bs.length; j++) {
        out.push(bs[j]);
      }
    }
    return out;
  };

const main = () => {
  console.log({ assLength: ass.length });

  {
    console.log("original start");
    try {
      const as = chainWithIndex((_, x) => x)(ass);
      console.log({ asLength: as.length });
    } catch (err: any) {
      console.log("[original][catch]", err.message);
    }
    console.log("original end");
  }

  {
    console.log("alternative start");
    const as = chainWithIndex2((_, x) => x)(ass);
    console.log({ asLength: as.length });
    console.log("alternative end");
  }
  console.log("done");
};

main();

Command used to run it:

ts-node --transpile-only x.ts

Output:

{ assLength: 1 }
original start
[original][catch] Maximum call stack size exceeded
original end
alternative start
{ asLength: 224402 }
alternative end
done

Prevent:
```
RangeError: Maximum call stack size exceeded
```

Simple example with code inlined to prevent imports.
```
/** Inlined from source code */
const isEmpty = <A>(as: ReadonlyArray<A>): as is readonly [] => as.length === 0;
// const empty: ReadonlyArray<never> = RNEA.empty
const empty: ReadonlyArray<never> = [];

const value = {};
// const a: Array<Array<typeof value>> = [new Array(112201).fill(value)];
/**
 * 112201, the number that failed in production and Apple  M1 Max.
 * I had to increase the number to see a similar failure on my desktop workstation hence the
 * `* 2`
 */
const ass: Array<Array<typeof value>> = [new Array(112201 * 2).fill(value)];

// chainWithIndex from ReadonlyArray
const chainWithIndex =
  <A, B>(f: (i: number, a: A) => ReadonlyArray<B>) =>
  (as: ReadonlyArray<A>): ReadonlyArray<B> => {
    if (isEmpty(as)) {
      return empty;
    }
    const out: Array<B> = [];
    for (let i = 0; i < as.length; i++) {
      out.push(...f(i, as[i]));
    }
    return out;
  };

// Alternative implementation
export const chainWithIndex2 =
  <A, B>(f: (i: number, a: A) => ReadonlyArray<B>) =>
  (as: ReadonlyArray<A>): ReadonlyArray<B> => {
    if (isEmpty(as)) {
      return [];
    }
    const out: Array<B> = [];
    for (let i = 0; i < as.length; i++) {
      const bs = f(i, as[i]);
      for (let j = 0; j < bs.length; j++) {
        out.push(bs[j]);
      }
    }
    return out;
  };

const main = () => {
  console.log({ assLength: ass.length });

  {
    console.log("original start");
    try {
      const as = chainWithIndex((_, x) => x)(ass);
      console.log(as.length);
    } catch (err: any) {
      console.log("[original][catch]", err.message);
    }
    console.log("original end");
  }

  {
    console.log("alternative start");
    const as = chainWithIndex2((_, x) => x)(ass);
    console.log({ asLength: as.length });
    console.log("alternative end");
  }
  console.log("done");
};

main();
```

Command used to run it:
```
ts-node --transpile-only x.ts
```

Output:
```
{ assLength: 1 }
original start
[original][catch] Maximum call stack size exceeded
original end
alternative start
{ asLength: 224402 }
alternative end
done
```
@gcanti
Copy link
Owner

gcanti commented Mar 25, 2024

Thanks @christiantakle

The issue lies with the use of Array.push in conjunction with the spread operator. There are three other instances of this problem. You can locate them by searching for ".push(...". Could you please address these occurrences as well?

Prevent `Maximum call stack size exceeded` exception when
`<array-b>` is large.
@christiantakle
Copy link
Contributor Author

@gcanti Done.

I do make a change to package-lock.json when running npm install assume its okay to include it here.

@christiantakle
Copy link
Contributor Author

christiantakle commented Mar 25, 2024

If you want I can merge the commits for the .push(... change or kill this pr and make a new one with that change just to reduce noise and co-locate import change. I was just being lazy and did the first commit from the github interface.

@christiantakle
Copy link
Contributor Author

Prefer #1931

@christiantakle christiantakle deleted the patch-2 branch March 25, 2024 16:44
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.

[ReadonlyArray.ts] flatten can trigger RangeError: Maximum call stack size exceeded
2 participants