source-academy / js-slang

Implementations of sublanguages of JavaScript, TypeScript, Scheme and Python
https://source-academy.github.io/source/
Apache License 2.0
66 stars 104 forks source link

CSE Machine: Avoid pushing unnecessary env instructions #1687

Closed DiligentPenguinn closed 5 months ago

DiligentPenguinn commented 5 months ago

Description

This PR implements simple logic to avoid pushing envInstr when not needed. Specifically, envInstr can be avoided if there are no environment-dependent commands on the control. A command is environment-dependent if its evaluation depends on the context given by the current environment. Implements and resolves #1682

Type of change

How to test

The following programs should run with no environment instructions being pushed

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 8786106267

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/cse-machine/utils.ts 20 21 95.24%
<!-- Total: 22 23 95.65% -->
Totals Coverage Status
Change from base Build 8700098795: 0.03%
Covered Lines: 10917
Relevant Lines: 12952

💛 - Coveralls
DiligentPenguinn commented 5 months ago

@martin-henz yes, I was thinking that further generalization would add extra complication to what is a pedagogical strategy. If this is desired then I can further work on it in the future to be more exhaustive.

martin-henz commented 5 months ago

The canAvoidInstr function is extremely inefficient and slows down the deployment each time by almost one hour! @RichDom2185 and @sayomaki commented:

the code is not very efficient at all, it seems; we can just early return instead of iterating through the entire control every single step of the cse machine

I'm pretty sure this alone accounts for most of the 40minutes

same thing for the switch case logic, lots of unnecessary computation

@sayomaki : consider using javascript's Array.some() or Array.every() instead

@RichDom2185 : yes that also works, no need for a for loop, though I think the logic can be improved further

because the same computation is being repeated at every step

it should have been memoised

just create a new field in the control item and calculate it once only instead of calculating n times for each item where n is the number of control items