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

fix: rewrite recordRefsOfVariable to avoid blowing the stack #443

Merged

Conversation

ToddFincannonEI
Copy link
Collaborator

@ToddFincannonEI ToddFincannonEI commented Mar 8, 2024

Fixes #442

I refactored the tail-recursive recordRefsOfVariable function as an iterative function with an auxiliary stack. Details in #442.

@chrispcampbell chrispcampbell changed the title refactor: rewrite recordRefsOfVariable to avoid blowing the stack fix: rewrite recordRefsOfVariable to avoid blowing the stack Mar 11, 2024
Copy link
Contributor

@chrispcampbell chrispcampbell left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fixes and performance improvements.

I verified that the code produces the same results as before for En-ROADS.

FWIW I quickly made an automated test for this. With a deep dependency tree (~50000 equations deep), the test fails as expected before your fix, and gets farther with your fix applied. But the test then fails due to blowing the stack in toposort, but that's a separate issue and apparently not affecting EPS yet so I won't worry about it for now. I'll add the test to the repo after this PR is merged (but skipped by default since it's slow to run).

  // Note that this test takes a while (> 8 seconds) to run, so is skipped by default
  it.skip('should work without exceeding stack limits when model has deep dependency tree', () => {
    const n = 50000
    let mdl = 'x0 = 1 ~~|\n'
    for (let i = 1; i <= n; i++) {
      mdl += `x${i} = x${i - 1} + 1 ~~|`
    }

    const code = readInlineModelAndGenerateC(mdl, {
      inputVarNames: ['x0'],
      outputVarNames: [`x${n}`]
    })

    for (let i = 0; i <= n; i++) {
      expect(code).toContain(`double _x${i};`)
    }
  })

@chrispcampbell chrispcampbell merged commit a2bbce9 into climateinteractive:main Mar 11, 2024
3 checks passed
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.

Model reader running out of stack with a large model
2 participants