Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

asl · 2024-11-01T12:47:14Z

Lots of cleanups and fixes here and there

asl · 2024-11-01T12:52:00Z

This is PR on top of #4971 for simplicity.

As mentioned in #4974, IR::Annotations incur a huge overhead, and in general does not make any sense: it's just another level of indirection. So, this just implements old FIXME, plus does some cleanup here and there.

The impact of # of allocations is huge.
Before:

After:

So, we're 10% less of memory allocations by both total size and number.

The runtime impact with GC disabled is huge as well:

Command	Mean [s]	Min [s]	Max [s]	Relative
`test/gtestp4c-nogc-main --gtest_filter=P4CParserUnroll.switch_20160512`	3.240 ± 0.048	3.195	3.422	1.10 ± 0.03
`test/gtestp4c-nogc --gtest_filter=P4CParserUnroll.switch_20160512`	2.948 ± 0.061	2.855	3.041	1.00

So, we're 10% faster here (!)

Runtime impact with GC on is much less visible due to huge GC overhead (1.5x):

Command	Mean [s]	Min [s]	Max [s]	Relative
`test/gtestp4c-main --gtest_filter=P4CParserUnroll.switch_20160512`	4.584 ± 0.099	4.312	4.897	1.02 ± 0.03
`test/gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512`	4.512 ± 0.072	4.309	4.649	1.00

asl · 2024-11-01T13:18:19Z

sanitizers are broken after #4989

ChrisDodd · 2024-11-06T06:32:45Z

ir/ir.def

@@ -154,7 +157,7 @@ class P4Control : Type_Declaration, INestedNamespace, ISimpleNamespace, IApply,

 /// A P4-16 action
 class P4Action : Declaration, ISimpleNamespace, IAnnotated, IFunctional {
-    optional Annotations        annotations = Annotations::empty;


Would we get most/all of the memory savings by just using inline here instead of getting rid of IR::Annotations completely? That might be a simpler change.

Not all, as IR::Annotations will be still be a Node and it will still be cloned. Though "invisible" now when outer node is cloned by itself. Plus, there was already a FIXME for this change. I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection? For convenience we can certainly do using Annotations = IR::Vector<IR::Annotation> here, yes.

Plus I already fixed couple of bugs when it was Annotations (being a Node) queried for the presence of an Annotation. Certainly the check returned always false. Now only IAnnotated objects could be queried about annotations.

In addition to this: usually we only have 1-2 annotations max (e.g. @name and that's it). So we can store them inline using e.g. absl::InlineVector as underlying storage. This would remove almost all Annotation-related memory allocations. But first we'd need to reduce the size of Allocation itself IMO

I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection?

I agree.

asl · 2024-11-06T09:05:53Z

I rebased the branch after abseil string helpers were merged in. Now it should show the changes more cleanly

vlstill

Looks good, thank you. I went over ir, midend and most of frontend changes (except mainly of P4-14).

ir/annotations.cpp

vlstill · 2024-11-07T06:13:45Z

ir/base.def

+    bool needsParsing : 1;
+
+    /// If this is true this is a structured annotation, and there are some
+    /// constraints on its contents.
+    bool structured : 1;


Does the bit-field really decrease size of the struct? Since there are the Vector/IndexedVector members, I would expect the alignment to be 64bit and therefore it would not really meatter if the 2 bools are 2 bits or 2 bytes. Or is there another reason to use bit-field?

I am planning to fix this in subsequent PRs :) We really need a discriminated union here as sizeof(Annotation) is terrible large. For now I just put it here not to forget.

vlstill · 2024-11-07T06:17:56Z

ir/ir.def

@@ -154,7 +157,7 @@ class P4Control : Type_Declaration, INestedNamespace, ISimpleNamespace, IApply,

 /// A P4-16 action
 class P4Action : Declaration, ISimpleNamespace, IAnnotated, IFunctional {
-    optional Annotations        annotations = Annotations::empty;


I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection?

I agree.

midend/flattenInterfaceStructs.h

frontends/p4/sideEffects.h

Signed-off-by: Anton Korobeynikov <[email protected]>

initializer_list while there Signed-off-by: Anton Korobeynikov <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

Lots of cleanups and fixes here and there Signed-off-by: Anton Korobeynikov <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

asl · 2024-11-09T07:20:36Z

validate-p4 fixes are in p4gauntlet/toz3#41

fruffy · 2024-11-09T17:53:24Z

@smolkaj fyi. This might influence the way you work with annotations.

fruffy · 2024-11-09T17:56:13Z

testdata/p4_16_samples_outputs/pna-action-selector-1.p4.spec

@@ -98,7 +98,7 @@ table as {
 	actions {
 		a1
 		a2
-		set_exception
+		set_exception @defaultonly


This actually changes the semantics of the program, what change caused this? It looks like this fixes a bug, but would be good to know where this was a problem.

It almost had to have been a change in the DPDK back end code that fixed this, but I haven't tried to narrow down which of those changes it was. The frontend and midend output files have this annotation before and after this PR.

This was an obvious bug IMO, see https://github.com/p4lang/p4c/pull/4992/files#diff-2dded02099b0c47be6b37c34d45999cbe59f2ed1e408e6fcc7b73455dff71caaL417

Essentially the checks there were always false as it was IR::Annotations that was queried, and not the original node.

vlstill · 2024-11-13T13:32:30Z

@asl, apparently this breaks dbprint of annotated types, e.g. header of a function now can look like this in dbprint: { }void foo({ }in int<16> bar, { }in int<16> baz, { }out int<16> boo). Presumably the problem is that if annotations member is just printed, it will be printed using IR::Vector::dbprint, which is printed as bracket-encodes list with , separators.

asl · 2024-11-13T13:38:18Z

@vlstill The debug printing of annotations was pretty broken before this PR. In particular it only prints expr, so unparsed or structured annotations are not printed at all(!).

I'm working on the proper fix when porting Annotation to disjoint union. Stay tuned :)

asl requested review from vlstill, ChrisDodd and fruffy November 1, 2024 12:47

asl force-pushed the annotations-clones branch from 497afd2 to aed00f8 Compare November 1, 2024 12:53

ChrisDodd reviewed Nov 6, 2024

View reviewed changes

asl force-pushed the annotations-clones branch from aed00f8 to 4a82acc Compare November 6, 2024 09:03

asl force-pushed the annotations-clones branch from 4a82acc to fa34687 Compare November 7, 2024 05:29

vlstill reviewed Nov 7, 2024

View reviewed changes

asl requested a review from vlstill November 8, 2024 07:05

vlstill approved these changes Nov 8, 2024

View reviewed changes

asl added 5 commits November 8, 2024 22:33

Simplify annotations query

cce4c5b

Signed-off-by: Anton Korobeynikov <[email protected]>

Rearrange the fields to reduce the size of object. Fix broken

4b457aa

initializer_list while there Signed-off-by: Anton Korobeynikov <[email protected]>

Reduce the overheads

9e3ddeb

Signed-off-by: Anton Korobeynikov <[email protected]>

Remove IR::Annotations and make IAnnotated to carry annotations inline.

1600e73

Lots of cleanups and fixes here and there Signed-off-by: Anton Korobeynikov <[email protected]>

Address review comments

78d5ed8

Signed-off-by: Anton Korobeynikov <[email protected]>

asl force-pushed the annotations-clones branch from 692b624 to 78d5ed8 Compare November 9, 2024 06:33

asl enabled auto-merge November 9, 2024 06:33

asl added this pull request to the merge queue Nov 9, 2024

Merged via the queue into p4lang:main with commit 6f7353f Nov 9, 2024
17 of 19 checks passed

asl deleted the annotations-clones branch November 9, 2024 09:00

fruffy mentioned this pull request Nov 9, 2024

Bump Compiler Version to 1.2.5.0. #5005

Merged

fruffy reviewed Nov 9, 2024

View reviewed changes

fruffy mentioned this pull request Nov 9, 2024

The Tofino back end is currently broken #5008

Closed

This was referenced Nov 10, 2024

Combo of duplication action name check and LocalizeAllActions fix #4975

Open

Annotations incur huge overhead #4974

Closed

psivanup mentioned this pull request Nov 13, 2024

Update tofino backend for IAnnotated interface changes psivanup/p4c#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

asl commented Nov 1, 2024

asl commented Nov 1, 2024 •

edited

Loading

asl commented Nov 1, 2024

ChrisDodd Nov 6, 2024 •

edited

Loading

asl Nov 6, 2024 •

edited

Loading

vlstill Nov 7, 2024

asl commented Nov 6, 2024

vlstill left a comment

vlstill Nov 7, 2024

asl Nov 7, 2024

vlstill Nov 7, 2024

asl commented Nov 9, 2024

fruffy commented Nov 9, 2024 •

edited

Loading

fruffy Nov 9, 2024 •

edited

Loading

jafingerhut Nov 9, 2024

asl Nov 9, 2024 •

edited

Loading

vlstill commented Nov 13, 2024

asl commented Nov 13, 2024

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Conversation

asl commented Nov 1, 2024

asl commented Nov 1, 2024 • edited Loading

asl commented Nov 1, 2024

ChrisDodd Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

asl Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

asl commented Nov 6, 2024

vlstill left a comment

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

asl Nov 7, 2024

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

asl commented Nov 9, 2024

fruffy commented Nov 9, 2024 • edited Loading

fruffy Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

jafingerhut Nov 9, 2024

Choose a reason for hiding this comment

asl Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

vlstill commented Nov 13, 2024

asl commented Nov 13, 2024

asl commented Nov 1, 2024 •

edited

Loading

ChrisDodd Nov 6, 2024 •

edited

Loading

asl Nov 6, 2024 •

edited

Loading

fruffy commented Nov 9, 2024 •

edited

Loading

fruffy Nov 9, 2024 •

edited

Loading

asl Nov 9, 2024 •

edited

Loading