Codegen improvements #549

bwrrp · 2022-10-13T10:41:55Z

This started out as implementing codegen for the not() function, but grew into a little bit more...

fixed fn:name function not returning empty string for empty sequence argument
refactored codegen to generate mostly expressions and automatically manage identifiers rather than having to assign identifiers to everything - this reduced the generated code types to value, generator (paths only) or statement (generator body)
refactored codegen to pass and combine PartialCompilationResults everywhere instead of plain strings
aligned conversion functions to spec terminology (effective boolean value, atomization) and checked for correctness
fixed several instances of variables not being passed / combined (made variables required to reduce errors)
fixed a performance issue where the child axis would call getChildNodes for each iteration of the generator - it now loops using getFirstChild and getNextSibling
fixed some cases where buckets were not passed correctly to axis domFacade calls
fixed some issues in type annotation
implemented general comparison for sequence vs single value in order to not regress for attribute = value comparisons (previously only accepted because multiplicity for the attribute axis was not annotated correctly)
implemented codegen support for the context item expression
implemented codegen support for the node() test
implemented codegen support for non-axis path steps (filterExpr)
implemented codegen support for calling custom functions
implemented codegen support for calling selected built-in functions
implemented native codegen for the not() function

Things to note:

Built-in functions use an allowlist, as codegen'ed function calls don't currently have the full staticContext, executionParameters etc.
I had to add an options argument to executeJavascriptCompiledXPath in order to pass currentContext (required by custom functions)
I disabled the no-shadowed-variables tslint rule, because I think renaming the callback argument in the many mapPartialCompilationResult to something other than the var being mapped makes the code confusing and may even invite errors where the callback body tries to use the unchecked expression instead of the argument.

This turns the generated code for a path expression into an actual generator function, which greatly simplifies keeping state between calls to next(). This also prevents calling getChildNodes for each step for each call to next(). Instead, the generated code now iterates over children using getFirstChild / getNextSibling.

@id

This restores the ability to codegen @id = 'value', which was lost with the earlier annotation fix.

Turns out this was required to make the Closure compiler happy

Support for this is incomplete, only a few argument / return types are supported and built-in functions use an allowlist as we don't have the complete execution context available.

bundlemon · 2022-10-13T10:43:19Z

BundleMon

Files updated (2)

Status	Path	Size	Limits
✅	fontoxpath.js	83.51KB (+1.28KB +1.55%)	-
✅	fontoxpath.esm.js	83.62KB (+1.27KB +1.54%)	-

Total files change +2.55KB +1.55%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

coveralls · 2022-10-13T10:51:10Z

Coverage increased (+0.3%) to 91.473% when pulling 01eb49e on codegen-not-function into 06b88ed on master.

That one doesn't need a generator, only a ternary expression

DrRataplan

Awesome work! Especially like the yield statements! Makes the code way easier to read.

Left a few small nitpicky comments

DrRataplan · 2022-10-14T08:48:26Z

src/jsCodegen/emitAxis.ts

+		mapPartialCompilationResult(nodesExpr, (nodesExpr) =>
+			acceptAst(
+				`const ${stepContextItemExpr.code} of ${nodesExpr.code}`,
+				{ type: GeneratedCodeBaseType.Statement },


Statement? Rather an iteratorHeader or something. But fine for now.

Yep, or rename Statement to NonExpressionCodeFragment or something... It's only used here so felt like overkill to introduce a separate type...

DrRataplan · 2022-10-14T09:18:06Z

src/jsCodegen/emitFunctionCallExpr.ts

 	) => PartialCompilationResult
 > = {
-	'local-name': emitLocalNameFunction,
-	name: emitNameFunction,
+	'local-name/0': emitLocalNameFunction,


nitpick, but I'd replace the / with #: to align with the functionLookup operator

DrRataplan · 2022-10-14T10:12:43Z

src/jsCodegen/runtimeLib.ts

+		args: UntypedExternalValue[],
+		options: Options | null
+	) => unknown;
+	XPDY0002: typeof XPDY0002;


Where is this used?

DrRataplan · 2022-10-14T10:13:12Z

src/jsCodegen/emitFunctionCallExpr.ts

+			acceptAst(contextItemExpr.code, { type: GeneratedCodeBaseType.Value }, [
+				...contextItemExpr.variables,
+				`if (${contextItemExpr.code} === undefined || ${contextItemExpr.code} === null) {
+					throw new Error('XPDY0002: The function which was called depends on dynamic context, which is absent.');


Why are you not using the runtimelib here?

To this and the above - error throwing was and still is a bit inconsistent - do you prefer to import all error helpers through runtimeLib or to turn everything into new Error('XP...')?

DrRataplan

Awesome! Let me know what the performance results do for the queries that are now supported by codegen!

bwrrp added 21 commits October 12, 2022 10:36

Implement codegen for the not() function

c248318

Fix buckets for type tests

abdf47c

Check function namespace and arity. Fix empty check in name functions.

84fb01c

Fix multiplicity of value and node comparisons

1d7fe6b

Clean up runtimeLib

b17fde6

Fix type check and a few tests

29c76ef

Remove unnecessary indirection for emitBaseExpr

76191e1

Fix fn:node for empty sequences

7aba083

Refactor codegen

42decff

Implement general compare for single item vs sequence

28533c9

This restores the ability to codegen @id = 'value', which was lost with the earlier annotation fix.

Restore emitBaseExpr indirection

7a03631

Turns out this was required to make the Closure compiler happy

Make tslint and old node happy

da83bb5

Implement calling registered functions

6287968

Support for this is incomplete, only a few argument / return types are supported and built-in functions use an allowlist as we don't have the complete execution context available.

Implement filterExpr

4960611

Implement contextItemExpr codegen and fix predicates in filterExpr

ab33d42

Pass options in codegen tests

d86df3b

Fix ordering in codegen for single-node axis

7790b40

Add more codegen unit tests

9003cfe

Remove unused Function GeneratedCodeBaseType

8c632d1

Remove old TODOs

3fd53da

bwrrp requested a review from DrRataplan October 13, 2022 10:41

bwrrp added 2 commits October 14, 2022 10:12

Emit simpler code for a single-self-step path expression

f367bd1

That one doesn't need a generator, only a ternary expression

Fix conversion of null node value to nodes array

70d514c

DrRataplan requested changes Oct 14, 2022

View reviewed changes

bwrrp added 2 commits October 14, 2022 14:18

PR feedback

3c8557e

Fix linter issue

000b00d

DrRataplan approved these changes Oct 14, 2022

View reviewed changes

[CodeFactor] Apply fixes

01eb49e

bwrrp merged commit 6f99b73 into master Oct 14, 2022

bwrrp deleted the codegen-not-function branch October 14, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codegen improvements #549

Codegen improvements #549

bwrrp commented Oct 13, 2022

bundlemon bot commented Oct 13, 2022 •

edited

Loading

coveralls commented Oct 13, 2022 •

edited

Loading

DrRataplan left a comment

DrRataplan Oct 14, 2022

bwrrp Oct 14, 2022

DrRataplan Oct 14, 2022

DrRataplan Oct 14, 2022

DrRataplan Oct 14, 2022

bwrrp Oct 14, 2022

DrRataplan left a comment

Codegen improvements #549

Codegen improvements #549

Conversation

bwrrp commented Oct 13, 2022

bundlemon bot commented Oct 13, 2022 • edited Loading

BundleMon

coveralls commented Oct 13, 2022 • edited Loading

DrRataplan left a comment

Choose a reason for hiding this comment

DrRataplan Oct 14, 2022

Choose a reason for hiding this comment

bwrrp Oct 14, 2022

Choose a reason for hiding this comment

DrRataplan Oct 14, 2022

Choose a reason for hiding this comment

DrRataplan Oct 14, 2022

Choose a reason for hiding this comment

DrRataplan Oct 14, 2022

Choose a reason for hiding this comment

bwrrp Oct 14, 2022

Choose a reason for hiding this comment

DrRataplan left a comment

Choose a reason for hiding this comment

bundlemon bot commented Oct 13, 2022 •

edited

Loading

coveralls commented Oct 13, 2022 •

edited

Loading