Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] ensure CDA result Array[Array[Byte]] can be GCed after dec… #13011

Merged
merged 3 commits into from
May 11, 2023

Conversation

tpoterba
Copy link
Contributor

@tpoterba tpoterba commented May 9, 2023

…oding

danking
danking previously requested changes May 9, 2023
@@ -2498,6 +2482,9 @@ class Emit[C](
cb.assign(stageName, stageName.concat("|").concat(dynamicID.asString.loadString(cb)))
})

val encRes = cb.newLocal[Array[Array[Byte]]]("encRes")
val len = mb.newLocal[Int]("cda_result_length")
val ib = mb.newLocal[InputBuffer]("decode_ib")
Copy link
Contributor

Choose a reason for hiding this comment

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

why sometimes class builder sometimes method builder?

Copy link
Contributor

Choose a reason for hiding this comment

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

More direct: How do you pick when to use mb vs cb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are aliased. Rewrote to use the more idiomatic "memoize" now and avoid explicit variable declarations.

The rule is basically to use the lowest-level builder that has the functionality you want. Module > Class > Method > Code, but codeBuilder doesn't duplicate the full functionality of everything above. So to create a field, use methodBuilder (it exists on MB and ClassB, not codeB).

.asBaseStruct
eltTupled.loadField(cb, 0)
}
cb.assign(encRes, Code._null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to also free each block after decoding it? As written we'd need twice the RAM to read a given result array, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice

@@ -2498,6 +2482,7 @@ class Emit[C](
cb.assign(stageName, stageName.concat("|").concat(dynamicID.asString.loadString(cb)))
})

val encRes = cb.newLocal[Array[Array[Byte]]]("encRes")
cb.assign(encRes, spark.invoke[BackendContext, HailClassLoader, FS, String, Array[Array[Byte]], Array[Byte], String, Option[TableStageDependency], Array[Array[Byte]]](
Copy link
Contributor

Choose a reason for hiding this comment

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

Approved, but is it possible to use memoize here as well or is this special in some way?

@danking danking merged commit b2afe57 into hail-is:main May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants