Performance improvement: Lookup table for State.actions #14

2023-09-08T01:57:19+02:00

nchen63 commented

2023-09-08 01:57:19 +02:00

(Migrated from github.com)

Caches State.actions in a lookup table, keyed by the task hash
- Improves performance from 145s to 57s in my grammar with the performance gain in optimizing the "collapse pass"

Fixes https://github.com/lezer-parser/lezer/issues/42

- Caches State.actions in a lookup table, keyed by the task hash - Improves performance from 145s to 57s in my grammar with the performance gain in optimizing the "collapse pass" Fixes https://github.com/lezer-parser/lezer/issues/42

marijnh (Migrated from github.com) reviewed

2023-09-11 08:25:14 +02:00

src/automaton.ts

This changes what this loop does—in the existing code, the action will be accepted (via continue actions) if any matching action is found. In the new code, the function will return false as soon as any conflicting action is found.

Given this distinction, I also don't think your assumption that the function will return the same for swapped a/b arguments is solid. It is probably possible to optimize that better than just calling it twice, though.

This changes what this loop does—in the existing code, the action will be accepted (via `continue actions`) if any matching action is found. In the new code, the function will return false as soon as any conflicting action is found. Given this distinction, I also don't think your assumption that the function will return the same for swapped `a`/`b` arguments is solid. It is probably possible to optimize that better than just calling it twice, though.

marijnh (Migrated from github.com) reviewed

2023-09-11 08:26:26 +02:00

src/automaton.ts

					
				@ -250,0 +262,4 @@

				      }

				    }

				    return this.actionsMapCache[term.hash] ?? []

				  }

It'd be nice if you tried to use spacing consistent with the rest of the file (I see 3 styles for for/if in this function, and no spaces around the = below).

It'd be nice if you tried to use spacing consistent with the rest of the file (I see 3 styles for `for`/`if` in this function, and no spaces around the `=` below).

nchen63 (Migrated from github.com) reviewed

2023-09-11 10:43:05 +02:00

src/automaton.ts

ok reverted

nchen63 (Migrated from github.com) reviewed

2023-09-11 10:43:16 +02:00

src/automaton.ts

					
				@ -250,0 +262,4 @@

				      }

				    }

				    return this.actionsMapCache[term.hash] ?? []

				  }

fixed

nchen63 (Migrated from github.com) reviewed

2023-09-11 21:52:15 +02:00

src/automaton.ts

For what it's worth, this change didn't affect my generator output. Is it possible that this change wouldn't affect the overall algorithm?

marijnh (Migrated from github.com) reviewed

2023-09-12 13:44:08 +02:00

src/automaton.ts

This will only make a difference for states with multiple actions for a token (GLR) that happen to be mergeable, so it is likely this change doesn't affect most grammars.

marijnh commented

2023-09-12 16:24:23 +02:00

(Migrated from github.com)

I've merged github.com/lezer-parser/generator@e5ad5d560b and replaced the rest of this patch with github.com/lezer-parser/generator@4e5afa07c6. Could you take a look and see how well the optimization works for your grammar?

I've merged https://github.com/lezer-parser/generator/commit/e5ad5d560b197f69eee5c3b1edc9e2b9561363a1 and replaced the rest of this patch with https://github.com/lezer-parser/generator/commit/4e5afa07c69ee392a6ac63b9fe271510e93d2240. Could you take a look and see how well the optimization works for your grammar?

sfc-gh-nchen commented

2023-09-12 17:49:44 +02:00

(Migrated from github.com)

It works great, down from 57s to 35s. Thanks so much

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

lezer/generator!14

Rows
Columns