⚡ Bolt: Optimize high-frequency engine simulation hot paths#14
Conversation
- Hoist enrichmentMap and getColdEnrichment to module scope to reduce GC pressure in updateTorque (~2000Hz). - Move initBattery to engine initialization to avoid redundant work in updateGFX (~60Hz). - Fix variable shadowing of dt in updateGFX. Co-authored-by: lonewolf0708 <51030924+lonewolf0708@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
combustionEngine.lua (2)
1281-1294:⚠️ Potential issue | 🟠 MajorWire the cold enrichment into the fuel calculation.
Operator, the helper now computes
coldStartEnrichment, but the value is discarded—tragic, wasteful! The hot path still pays for the lookup, and cold-start fueling remains unchanged.🐛 Proposed fix
- local baseFuelAmount = 0.02 -- Base fuel scaled by enrichment + local baseFuelAmount = 0.02 * coldStartEnrichment -- Base fuel scaled by enrichmentIf enrichment is not intended yet, remove the call until it is used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combustionEngine.lua` around lines 1281 - 1294, coldStartEnrichment is being computed by getColdEnrichment(engineTempC) but never used; update the fuel calculation in the cranking/hot path to incorporate it (or remove the call). Specifically, in the block that computes final fuel for cranking/idle (use symbols baseFuelAmount, crankingFuelMultiplier, chokeEffect, chokeThrottleBoost and isCranking/engineTempC to find the correct spot), apply the enrichment by multiplying or adding coldStartEnrichment into the fuel amount (e.g., scale baseFuelAmount or apply to crankingFuelMultiplier) so cold starts get richer fuel; alternatively remove the unused getColdEnrichment call if enrichment is intentionally deferred.
647-666:⚠️ Potential issue | 🟠 MajorUse the real frame delta for battery charge and drain.
Renaming the shadowed value is good, but
batteryDt = 1/60still makes battery state depend on how oftenupdateGFXactually runs. Frame drops, slow motion, or a non-60Hz update cadence will skew drain/recharge.🐛 Proposed fix
- local batteryDt = 1/60 -- Fixed timestep for battery updates + local batteryDt = dt -- Use the actual update interval for battery updates🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combustionEngine.lua` around lines 647 - 666, The battery update uses a hardcoded batteryDt = 1/60 which makes charge/drain wrong when the update cadence changes; replace that constant with the real frame delta provided by the update loop (e.g., the dt/deltaTime parameter passed into updateGFX or the engine update function) and use it (in seconds) in the battery math where batteryDt is referenced; update logic around device.batteryCharge, device.batteryDrainRate, device.batteryChargeRate, device.outputAV1, device.idleAV, device.starterEngagedCoef and device.starterMaxAV to use the actual dt (converting units if the loop supplies milliseconds) so drainRate/chargeRate are scaled correctly by elapsed time instead of a fixed 1/60.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@combustionEngine.lua`:
- Around line 39-81: The interpolation sentinels in getColdEnrichment are wrong:
lowerTemp/upperTemp and their enrich defaults don't reflect actual enrichmentMap
entries (causing -20..-10 to interpolate against wrong values and using a hidden
80→0.85 point). Fix by either (A) making the 80→0.85 point explicit in
enrichmentMap, or (B) change getColdEnrichment to initialize lowerTemp/upperTemp
and lowerEnrich/upperEnrich from the actual first/last keys of enrichmentMap (or
use -math.huge/+math.huge sentinels) and then pick the nearest lower/upper
entries during the pairs() scan so the enrich values always come from
enrichmentMap entries (use enrichmentMap[temp] to set lowerEnrich/upperEnrich
when updating lowerTemp/upperTemp).
---
Outside diff comments:
In `@combustionEngine.lua`:
- Around line 1281-1294: coldStartEnrichment is being computed by
getColdEnrichment(engineTempC) but never used; update the fuel calculation in
the cranking/hot path to incorporate it (or remove the call). Specifically, in
the block that computes final fuel for cranking/idle (use symbols
baseFuelAmount, crankingFuelMultiplier, chokeEffect, chokeThrottleBoost and
isCranking/engineTempC to find the correct spot), apply the enrichment by
multiplying or adding coldStartEnrichment into the fuel amount (e.g., scale
baseFuelAmount or apply to crankingFuelMultiplier) so cold starts get richer
fuel; alternatively remove the unused getColdEnrichment call if enrichment is
intentionally deferred.
- Around line 647-666: The battery update uses a hardcoded batteryDt = 1/60
which makes charge/drain wrong when the update cadence changes; replace that
constant with the real frame delta provided by the update loop (e.g., the
dt/deltaTime parameter passed into updateGFX or the engine update function) and
use it (in seconds) in the battery math where batteryDt is referenced; update
logic around device.batteryCharge, device.batteryDrainRate,
device.batteryChargeRate, device.outputAV1, device.idleAV,
device.starterEngagedCoef and device.starterMaxAV to use the actual dt
(converting units if the loop supplies milliseconds) so drainRate/chargeRate are
scaled correctly by elapsed time instead of a fixed 1/60.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 717989f7-fc3c-4c45-bbf8-79d6f5a07287
📒 Files selected for processing (2)
.jules/bolt.mdcombustionEngine.lua
| -- Static enrichment map to avoid re-allocation in the hot path | ||
| local enrichmentMap = { | ||
| [-30] = 3.0, | ||
| [-20] = 2.6, | ||
| [-10] = 2.2, | ||
| [0] = 1.8, | ||
| [10] = 1.5, | ||
| [20] = 1.3, | ||
| [30] = 1.15, | ||
| [40] = 1.05, | ||
| [50] = 1.02, | ||
| [60] = 1.0, | ||
| [70] = 1.0 | ||
| } | ||
|
|
||
| -- Optimized cold start enrichment calculation moved to module scope | ||
| local function getColdEnrichment(tempC) | ||
| -- Find the two closest temperature points | ||
| local lowerTemp = -20 | ||
| local upperTemp = 80 | ||
| local lowerEnrich = 3.0 | ||
| local upperEnrich = 0.85 | ||
|
|
||
| -- Find the two closest temperature points in the map | ||
| for temp, enrich in pairs(enrichmentMap) do | ||
| if temp <= tempC and temp > lowerTemp then | ||
| lowerTemp = temp | ||
| lowerEnrich = enrich | ||
| end | ||
| if temp >= tempC and temp < upperTemp then | ||
| upperTemp = temp | ||
| upperEnrich = enrich | ||
| end | ||
| end | ||
|
|
||
| -- Linear interpolation between the two closest points | ||
| if lowerTemp == upperTemp then | ||
| return lowerEnrich | ||
| end | ||
|
|
||
| local t = (tempC - lowerTemp) / (upperTemp - lowerTemp) | ||
| return lowerEnrich + (upperEnrich - lowerEnrich) * t | ||
| end |
There was a problem hiding this comment.
Fix the cold-enrichment interpolation bounds.
Operator, the sentinels are... confused. lowerTemp = -20 starts with lowerEnrich = 3.0, so temperatures between -20 and -10 interpolate from the wrong enrichment. Values above 70 also use an implicit 80 → 0.85 point that is not in the map.
🐛 Proposed fix
--- Static enrichment map to avoid re-allocation in the hot path
-local enrichmentMap = {
- [-30] = 3.0,
- [-20] = 2.6,
- [-10] = 2.2,
- [0] = 1.8,
- [10] = 1.5,
- [20] = 1.3,
- [30] = 1.15,
- [40] = 1.05,
- [50] = 1.02,
- [60] = 1.0,
- [70] = 1.0
+-- Static enrichment points to avoid re-allocation in the hot path
+local enrichmentPoints = {
+ {-30, 3.0},
+ {-20, 2.6},
+ {-10, 2.2},
+ {0, 1.8},
+ {10, 1.5},
+ {20, 1.3},
+ {30, 1.15},
+ {40, 1.05},
+ {50, 1.02},
+ {60, 1.0},
+ {70, 1.0}
}
-- Optimized cold start enrichment calculation moved to module scope
local function getColdEnrichment(tempC)
- -- Find the two closest temperature points
- local lowerTemp = -20
- local upperTemp = 80
- local lowerEnrich = 3.0
- local upperEnrich = 0.85
-
- -- Find the two closest temperature points in the map
- for temp, enrich in pairs(enrichmentMap) do
- if temp <= tempC and temp > lowerTemp then
- lowerTemp = temp
- lowerEnrich = enrich
- end
- if temp >= tempC and temp < upperTemp then
- upperTemp = temp
- upperEnrich = enrich
- end
+ if tempC <= enrichmentPoints[1][1] then
+ return enrichmentPoints[1][2]
end
- -- Linear interpolation between the two closest points
- if lowerTemp == upperTemp then
- return lowerEnrich
+ for i = 2, `#enrichmentPoints` do
+ local upperPoint = enrichmentPoints[i]
+ if tempC <= upperPoint[1] then
+ local lowerPoint = enrichmentPoints[i - 1]
+ local t = (tempC - lowerPoint[1]) / (upperPoint[1] - lowerPoint[1])
+ return lowerPoint[2] + (upperPoint[2] - lowerPoint[2]) * t
+ end
end
- local t = (tempC - lowerTemp) / (upperTemp - lowerTemp)
- return lowerEnrich + (upperEnrich - lowerEnrich) * t
+ return enrichmentPoints[`#enrichmentPoints`][2]
endIf the 80°C → 0.85 behavior is intentional, make it an explicit point instead of a hidden default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@combustionEngine.lua` around lines 39 - 81, The interpolation sentinels in
getColdEnrichment are wrong: lowerTemp/upperTemp and their enrich defaults don't
reflect actual enrichmentMap entries (causing -20..-10 to interpolate against
wrong values and using a hidden 80→0.85 point). Fix by either (A) making the
80→0.85 point explicit in enrichmentMap, or (B) change getColdEnrichment to
initialize lowerTemp/upperTemp and lowerEnrich/upperEnrich from the actual
first/last keys of enrichmentMap (or use -math.huge/+math.huge sentinels) and
then pick the nearest lower/upper entries during the pairs() scan so the enrich
values always come from enrichmentMap entries (use enrichmentMap[temp] to set
lowerEnrich/upperEnrich when updating lowerTemp/upperTemp).
1 participant
💡 What:
🎯 Why:
📊 Impact:
🔬 Measurement:
PR created automatically by Jules for task 7832147704157988298 started by @lonewolf0708
Summary by CodeRabbit
Documentation
Refactor