Skip to content

Conversation

@ike709
Copy link
Collaborator

@ike709 ike709 commented Sep 8, 2025

Nukes DMASTFolder. It's deprecated and removing it shaves a consistent 6 seconds off my third-run TG compile time (45s -> 39s).

This uncovered an OD bug in the following preprocessed TG code:

/proc/collect_vv(obj/item/item)
    var/static/list/ignored_vars = list(
        (@#animate_movement# ||item.animate_movement),
        (@#datum_flags# ||item.datum_flags),
        (@#fingerprintslast# ||item.fingerprintslast),
        (@#layer# ||item.layer),
        (@#plane# ||item.plane),
        (@#screen_loc# ||item.screen_loc),
        (@#tip_timer# ||item.tip_timer),
        (@#vars# ||item.vars),
        (@#x# ||item.x),
        (@#y# ||item.y),
        (@#z# ||item.z),
    )

DMASTFolder was returning just the LHS as a constant string, which hid the fact that OD couldn't resolve the deref in the RHS. I think I got it working, but BuildIdentifier() is such a mess I'm not 100% sure this is the proper fix.

@ike709 ike709 requested a review from wixoaGit September 8, 2025 09:25
@boring-cyborg boring-cyborg bot added the Compiler Involves the OpenDream compiler label Sep 8, 2025
@github-actions github-actions bot added the size/L label Sep 8, 2025
@ike709
Copy link
Collaborator Author

ike709 commented Sep 8, 2025

I diffed the disassembly of TG before and after these changes and the bytecode is functionally equivalent, except we now include boolean expressions that had previously been const folded out:
image
image
I think it's fine for now; I intend to revisit const-folding after this PR is merged anyways. The improved compiletime is worth a few useless opcodes.

@ike709
Copy link
Collaborator Author

ike709 commented Sep 8, 2025

Also confirmed I can observe on TG without things being horribly broken.

private DMExpression BuildIdentifier(DMASTIdentifier identifier, DreamPath? inferredPath = null) {
var name = identifier.Identifier;
if (scopeMode == Normal) {
if (scopeMode == Normal || scopeMode == Static) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid even in BYOND, local vars/args aren't available to static vars. Making this change will cause the global init proc to attempt to use one of its own locals, which is incorrect.

I think what BYOND is doing here is a first check that the var exists when the expression is created (that is ignoring the static context), and a second check when the bytecode is generated that is being skipped because of constant folding. As far as I can tell, the first step ignoring the static context is a bug. I'm not sure of the best way to replicate this with the way OD is structured.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not know how to fix this.

@ike709 ike709 marked this pull request as draft September 15, 2025 17:35
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Compiler Involves the OpenDream compiler Merge Conflict size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants