Repository navigation
Conversation
e880006 to
9cedbce
Compare
Remove extractor-imposed Node heap limits and obsolete TypeScript RAM configuration. Stop interpreting `CODEQL_RAM` in the JavaScript autobuild launchers, but retain `--typescript-ram` as an ignored compatibility option. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7f45b794-38b9-4abc-b652-03279c8dba77
9cedbce to
ac7cb39
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the extractor memory-limit changes.
No open issues 🎉
What changed in this PR
Removes extractor-imposed memory limits for JavaScript and TypeScript extraction, reflecting the TypeScript process’s reduced role of parsing without type checking.
Changes:
- Removes the Node.js heap-limit override and associated TypeScript memory configuration.
- Stops splitting
CODEQL_RAMbetween Java and Node.js in both autobuild scripts. - Keeps
--typescript-ramaccepted as a deprecated, ignored option.
| File | Description |
|---|---|
javascript/resources/tools/autobuild.sh |
Removes RAM splitting and the Java heap override on Unix. |
javascript/resources/tools/autobuild.cmd |
Removes equivalent memory configuration on Windows. |
javascript/extractor/src/com/semmle/ts/extractor/TypeScriptParser.java |
Removes RAM settings and the explicit Node.js heap limit. |
javascript/extractor/src/com/semmle/js/extractor/Main.java |
Accepts but ignores the deprecated --typescript-ram option. |
javascript/extractor/src/com/semmle/js/extractor/ExtractorConfig.java |
Removes the TypeScript RAM field and accessors. |
Review effort: Balanced (auto)
|
I'd like to wait for the QA run before merging this. In any case I'm not sure I see the need for this change tbh. These flags exist because Node and Java have a self-enforced max heap limit which need to be high enough. Why rely on the default value being high enough? Go doesn't have such a limit and we wouldn't need to set anything similar for the Go subprocess. It's not a "hard to enforce limit" it's more like "this problem doesn't exist for Go". |
|
Thanks @asgerf. Perhaps I was focusing too much on the comment saying:
If we wanted to implement the limit like stated there, then it would be hard, because most of that RAM is now used in the Go process. But I see your point. Perhaps, instead of this PR, we should just change the comments and docs to make it clear that it's about setting the memory limit for the NodeJS process and not the TypeScript compiler? |
Remove memory limits imposed on the NodeJS process by the Java process. Rationale copy/pasted from the internal issue:
This also removes the
CODEQL_RAM/2 memory limit on the Java process. I took a look, and the only other extractor that usesCODEQL_RAMis the Java extractor. So I think ignoring that here is perfectly fine.