[Mono] Use dn_vector_t instead of GArray in a few places #84027

lambdageek · 2023-03-28T15:30:19Z

Set up the infrastructure to consume shared container types from src/mono/{utils,metadata,containers,mini}.

There are about 20 total places where we use GArray. Replace a few of them with dn_vector_t as an experiment to see how using shared containers in Mono will look like.

Set up the infrastructure to consume shared container types from src/mono/{metadata,mini}. Replace one use of GArray with dn_vector_t

lambdageek · 2023-03-28T15:30:36Z

fyi @lateralusX

src/mono/mono/metadata/CMakeLists.txt

src/mono/mono/metadata/custom-attrs.c

Use a stack allocated container (with heap-allocated data) to avoid one extra allocation

Add a way to specify target_link_libraries for runtime components. Use it to link in the shared containers. Avoids duplicate linking of the container objects in static linking scenarios.

lambdageek · 2023-03-28T20:40:26Z

This is just a start. There are around 20 uses of g_array_new... in Mono, 89 g_ptr_array_new..., 7 g_byte_array_new.

And after that there's 289 g_hash_table_new... 😱

azure-pipelines · 2023-04-05T23:51:56Z

Azure Pipelines successfully started running 1 pipeline(s).

fix windows include problems, I guess?

lambdageek · 2023-04-06T15:36:16Z

@LakshanF could you take a look at the nativeaot (and coreclr?) bit. There's just one thing here:

I'm adding a checked fast fail macro dn_checkfail(cond,msg,...) that checks if cond is true and if not, fast fails (in both release and debug builds). It's not actually used inside the containers library anywhere, but I want to start using it in Mono as we migrate to the shared containers to preserve our old behavior (unlike asserts in coreclr, mono's g_assert macro fires in both debug and release builds).

The nativeaot version just ignores the message for now and calls RhFastFail. I could try to make it print out the message, but I wasn't sure if there's some Redhawk way of doing that or what the general policy in native aot is around diagnostic output of this sort.

lateralusX · 2023-04-19T13:16:26Z

src/mono/mono/containers/dn-rt-utils.c

@@ -0,0 +1,19 @@
+// Licensed to the .NET Foundation under one or more agreements.


Shouldn't the naming of this file follow the naming of the header, dn-rt-mono.c?

lateralusX · 2023-04-19T17:25:18Z

src/mono/mono/mini/debug-mini.c

@@ -78,7 +79,8 @@ mono_debug_open_method (MonoCompile *cfg)
 	g_assert (header);

 	info->jit = jit = g_new0 (MonoDebugMethodJitInfo, 1);
-	info->line_numbers = g_array_new (FALSE, TRUE, sizeof (MonoDebugLineNumberEntry));
+	info->line_numbers = dn_vector_alloc_t (MonoDebugLineNumberEntry);
+	dn_checkfail (info->line_numbers != NULL, "Allocation failed");


Thinking a little about this pattern, could an alternative be to have a new vector attribute that we can set when doing custom alloc/init that we fail fast internal, like DN_VECTOR_ATTRIBUTE_FAIL_FAST. That code path is already in the slow path so check this attribute and fail fast if requested instead of returning NULL should not add overhead to fast path. It will be an opt in and you need to use the custom init/alloc to get it, but will reduce need to check returns due to allocation failures in code that doesn't do it. Failures could happen to a number of different functions, alloc/init/push_back/resize etc.

We should still keep option to use dn_checkfail as well of course, but adding the attribute would make it possible to align the type to current Mono allocation failure behaviors.

We could also add a couple of pre defined init structs that enable some values, so instead of needing to declare a struct with this flag set, you can just use the predeclared struct in your custom init/alloc call.

Honestly that all sounds like a huge pain.

I would prefer something like

dn_vector_t *vec = DN_AOK (dn_vector_alloc_t (...));

with something like

#define DN_AOK(expr) ({ void *_ck = (expr); dn_checkfail(_ck != NULL, "Allocation failed"); _ck})

except:

with proper macros so the file:line is from the caller of DN_AOK(), not from the macro definiton

with some typeof trick so the whole expression has the same type as expr

without using the GCC ({ stmt; expr }) non-standard extension

I think it could be rather straightforward, and it would simplify case to work similar to how g_array works where it asserts on internal allocation failures, so you could make sure a dn_vector_t would behave the same way with regards to internal allocation failures.

The macro way works as well, but they are not exclusive, both variations could exist. Handle it using macros will of course generate more code around each call to a vector function that could fail (that we didn't check for failures when using g_array), but maybe its good to set the pattern checking return values for the new type right away, instead of offering a lazy fallback. The macro solution would probably be able to validate both pointer as well as bool return cases for failures.

lateralusX · 2023-04-19T17:31:53Z

src/mono/mono/mini/interp/transform.c

@@ -3939,7 +3941,8 @@ recursively_make_pred_seq_points (TransformData *td, InterpBasicBlock *bb)
 {
 	SeqPoint ** const MONO_SEQ_SEEN_LOOP = (SeqPoint**)GINT_TO_POINTER(-1);

-	GArray *predecessors = g_array_new (FALSE, TRUE, sizeof (gpointer));
+	dn_vector_ptr_t predecessors = {0,};


No need to init the dn_vector_ptr_t, it will be memset by dn_vector_ptr_init.

I'm not sure if C compilers (or linters) would like that.

and I also don't particularly like it

It's not an uncommon pattern that you have an unutilized, or partly initialized struct that you pass to an init function setting up the values, avoiding setting the same memory multiple times. I agree that its cleaner code to always initialize things meaning you won't end up with potential initialized memory in the end, just wanted to point out that the init function makes sure full struct will be set, so in this case we will overwrite the same memory multiple times.

lateralusX · 2023-04-19T17:32:18Z

src/mono/mono/mini/seq-points.c

@@ -33,7 +35,9 @@ recursively_make_pred_seq_points (MonoCompile *cfg, MonoBasicBlock *bb)
 {
 	const gpointer MONO_SEQ_SEEN_LOOP = GINT_TO_POINTER(-1);

-	GArray *predecessors = g_array_new (FALSE, TRUE, sizeof (gpointer));
+
+	dn_vector_ptr_t predecessors = {0,};


No need to init the dn_vector_ptr_t, it will be memset by dn_vector_ptr_init.

lateralusX · 2023-04-19T17:33:01Z

src/mono/mono/metadata/seq-points-data.c

@@ -274,24 +275,27 @@ mono_seq_point_init_next (MonoSeqPointInfo* info, SeqPoint sp, SeqPoint* next)
 	int i;
 	guint8* ptr;
 	SeqPointIterator it;
-	GArray* seq_points = g_array_new (FALSE, TRUE, sizeof (SeqPoint));
+	dn_vector_t seq_points = {0,};


No need to init the dn_vector_t, it will be memset by dn_vector_init_t.

lambdageek · 2023-04-20T14:08:34Z

Just to give a brief update: I'm probably going to pause on this until Preview 4 is out.

Also in the follow-up branch where I try to get rid of all uses of GPtrArray I found a few more cmake tweaks that I need to make. I will bring those over to this PR, too, so we have all the build stuff sorted out.

lateralusX · 2023-05-23T08:01:06Z

Still planning on getting this in (P6)? If not, it would at least be nice to get the additional defines in src/native/containers/dn-vector-ptr.h included from this PR.

lambdageek · 2023-05-23T15:03:52Z

@lateralusX I'll get back to this next week. hope I can at least get the basic infrastructure in place

ghost · 2023-06-22T17:02:17Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

Use dn_vector_t instead of GArray in one place in Mono

5f0bb98

Set up the infrastructure to consume shared container types from src/mono/{metadata,mini}. Replace one use of GArray with dn_vector_t

ghost assigned lambdageek Mar 28, 2023

dotnet-issue-labeler bot added the area-Build-mono label Mar 28, 2023

lambdageek commented Mar 28, 2023

View reviewed changes

src/mono/mono/metadata/CMakeLists.txt Outdated Show resolved Hide resolved

src/mono/mono/metadata/custom-attrs.c Outdated Show resolved Hide resolved

lambdageek added 4 commits March 28, 2023 12:17

include shared containers in mono shared lib, too

a991249

convert mono_seq_point_init_next to use dn_vector_t

87f5d50

[custom-attrs] Use init/dispose instead of alloc/free for dn_vector_t

e8c7dea

Use a stack allocated container (with heap-allocated data) to avoid one extra allocation

move container logic to toplevel; add deps to runtime components

2c5c00a

Add a way to specify target_link_libraries for runtime components. Use it to link in the shared containers. Avoids duplicate linking of the container objects in static linking scenarios.

lambdageek changed the title ~~[Mono] Use dn_vector_t instead of GArray in one place~~ [Mono] Use dn_vector_t instead of GArray in a few places Mar 28, 2023

lambdageek added 5 commits March 28, 2023 14:31

fix windows build

301c2a4

fix Darwin frameworks builds

ec1bf21

replace 2 uses of GArray in interp/transform.c

473df84

replace a use of GArray in mini/seq-points.c

5e4b453

replace use of GArray for debug-mini

6029c33

lambdageek force-pushed the use-shared-containers-in-mono branch from ab17450 to 6029c33 Compare March 28, 2023 20:38

This was referenced Mar 29, 2023

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

[release/6.0] Doublelinklist GC failures on Mono #83245

Closed

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

Wasm debugger test timing out #83847

Closed

lambdageek marked this pull request as ready for review March 29, 2023 16:13

lambdageek requested review from BrzVlad, vargaz, kotlarmilos, SamMonoRT, thaystg and marek-safar as code owners March 29, 2023 16:13

lambdageek added area-VM-meta-mono and removed area-Build-mono labels Mar 29, 2023

lambdageek force-pushed the use-shared-containers-in-mono branch from b567e8b to 3bffcf2 Compare April 6, 2023 00:39

add placeholder dn_rt functions for coreclr and nativeaot

71fb5c2

lambdageek force-pushed the use-shared-containers-in-mono branch from 3bffcf2 to 71fb5c2 Compare April 6, 2023 02:16

fix mono build

fa05fbf

lambdageek force-pushed the use-shared-containers-in-mono branch from 5bb2c06 to fa05fbf Compare April 6, 2023 02:30

lambdageek added 2 commits April 5, 2023 23:30

maybe fix coreclr/nativeaot windows build?

695575f

move nativeaot shims to a separate file

c4a28a5

fix windows include problems, I guess?

lambdageek requested a review from MichalStrehovsky as a code owner April 6, 2023 04:26

fix coreclr windows?

e0526c1

This was referenced Apr 6, 2023

Tracking issue for CI build timeouts #76454

Closed

StackallocTests.Test4096 failing EnsureZeroed check #84398

Open

lambdageek added 2 commits April 6, 2023 11:03

add typed dn_vector_ptr accessor macros

7fc5ecc

Use dn_vector_ptr in a few places instead of dn_vector

e93efdc

lambdageek requested a review from lateralusX April 6, 2023 15:14

lambdageek requested a review from LakshanF April 6, 2023 15:54

lambdageek mentioned this pull request Apr 7, 2023

[draft] Completely replace GArray by dn_vector_t in Mono #84465

Closed

lateralusX reviewed Apr 19, 2023

View reviewed changes

lambdageek marked this pull request as draft May 23, 2023 15:04

ghost closed this Jun 22, 2023

ghost locked as resolved and limited conversation to collaborators Jul 22, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mono] Use dn_vector_t instead of GArray in a few places #84027

[Mono] Use dn_vector_t instead of GArray in a few places #84027

lambdageek commented Mar 28, 2023 •

edited

Loading

lambdageek commented Mar 28, 2023

lambdageek commented Mar 28, 2023

azure-pipelines bot commented Apr 5, 2023

lambdageek commented Apr 6, 2023 •

edited

Loading

lateralusX Apr 19, 2023

lateralusX Apr 19, 2023 •

edited

Loading

lateralusX Apr 19, 2023

lambdageek Apr 20, 2023

lateralusX Apr 20, 2023 •

edited

Loading

lateralusX Apr 19, 2023

lambdageek Apr 20, 2023 •

edited

Loading

lateralusX Apr 20, 2023

lateralusX Apr 19, 2023

lateralusX Apr 19, 2023

lambdageek commented Apr 20, 2023

lateralusX commented May 23, 2023

lambdageek commented May 23, 2023

ghost commented Jun 22, 2023

		@@ -0,0 +1,19 @@
		// Licensed to the .NET Foundation under one or more agreements.

[Mono] Use dn_vector_t instead of GArray in a few places #84027

[Mono] Use dn_vector_t instead of GArray in a few places #84027

Conversation

lambdageek commented Mar 28, 2023 • edited Loading

lambdageek commented Mar 28, 2023

lambdageek commented Mar 28, 2023

azure-pipelines bot commented Apr 5, 2023

lambdageek commented Apr 6, 2023 • edited Loading

lateralusX Apr 19, 2023

Choose a reason for hiding this comment

lateralusX Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

lateralusX Apr 19, 2023

Choose a reason for hiding this comment

lambdageek Apr 20, 2023

Choose a reason for hiding this comment

lateralusX Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

lateralusX Apr 19, 2023

Choose a reason for hiding this comment

lambdageek Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

lateralusX Apr 20, 2023

Choose a reason for hiding this comment

lateralusX Apr 19, 2023

Choose a reason for hiding this comment

lateralusX Apr 19, 2023

Choose a reason for hiding this comment

lambdageek commented Apr 20, 2023

lateralusX commented May 23, 2023

lambdageek commented May 23, 2023

ghost commented Jun 22, 2023

lambdageek commented Mar 28, 2023 •

edited

Loading

lambdageek commented Apr 6, 2023 •

edited

Loading

lateralusX Apr 19, 2023 •

edited

Loading

lateralusX Apr 20, 2023 •

edited

Loading

lambdageek Apr 20, 2023 •

edited

Loading