-
Notifications
You must be signed in to change notification settings - Fork 7
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
implement deflate_if
conditional compression
#26
base: master
Are you sure you want to change the base?
Conversation
I've tested
|
Also, I would suggest squash & merge when merging PRs to avoid adding unmeaningful and verbose commits. |
Would it be a bit ambiguous to write |
Yes, that is what I thought as well.
|
Well, a combination of |
originally I wanted to suggest changing |
}; | ||
$crate::decode_string($crate::codegen::deflate_utf8_file!($path $($algo)?), Some($crate::CompressionMethodTy(algo))) | ||
// Evaluate the condition at compile time to avoid unnecessary runtime checks | ||
if $crate::codegen::deflate_if!($path $($algo)? $($($threshold)+)?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we actually use conditional compilation here? While the compiler should optimize away the constant false branch, it seems weird that we are including the file into the program twice just so that we could remove it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. can we make deflate_if!
emit all()
for true and any()
for false instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Unfortunately, the conditional compilation does not support such statements.
This (relying on compiler optimization with constant boolean statement) is the only way to achieve this without huge code refactor as described in the PR description.
Although it is still possible by either one of:
- making
flate!
itself a proc-macro. - make a proc-macro in codegen crate that generates
- if the
deflate_if!
returns true:$(pub $(($($vis)+))?)? static $name: $crate::Lazy<::std::vec::Vec<u8>> = $crate::Lazy::new(...)
; or
- if the
deflate_if!
returns false:$(pub $(($($vis)+))?)? static $name: &[u8] = include_str!(...)
.
- if the
Either ways are much more huge refactor/codebase change than just let the compiler optimize out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems weird that we are including the file into the program twice just so that we could remove it later.
Please note that deflate_if!
does not actually deflates the file. It is used for deciding whether or not the compression ratio meets the given threshold criteria. So the only a single file will be embedded in the binary as a result of compiler optimization, not the twice.
/// This macro evaluates to `true` if the file should be compressed, `false` otherwise, at compile time. | ||
/// Useful for conditional compilation without any efforts to the runtime. | ||
/// | ||
/// Please note that unlike the macro names suggest, this macro does **not** actually compress the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only expected for internal use, should we #[doc(hidden)]
the re-export? And shall we rename this to should_deflate
, since this does not actually do the imperative deflate
as the name suggests?
This PR introduces conditional compression
deflate_if
as @SOF3 pointed out in previous PR.Features
deflate_if!
proc-macro ininclude-flate-codegen
.The
deflate_if!
macro is completely isolated from thedeflate_file!
(ordeflate_utf8_file!
). This is by design. Because we do not actually want to (and should not) calculate/evaluate whether or not the file is actually compressed, at runtime.deflate_if!
is a proc-macro evaluates and returns boolean at compile-time in theLazy::new
. This constant boolean will trigger the compiler optimization and the compiler will remove the unreachable code, so it allows us to implement this feature without adding any efforts on the runtime code itself.Indeed, even if the
deflate_if!
evaluated tofalse
and entire decompression code is removed by compiler,once_cell::Lazy
and its related runtime codes will remain but it's not considerable to performance critical. We may want to make it use pure (not withLazy::new
)include_bytes!
orinclude_str!
if the compression should not be proceeded. However, to make this feature true, it requires huge refactor of core design of this crate itself. I decided this is not worth doing compared to the what we will achieve in this PR.Bug Fixes
flate!
macro other thanstr
types.macro_rules!
parameters into proc-macro implementation, but it wasn't raise any errors since the parameter is entiely optional. I've added test for this to ensure this never happen.Tests
deflate_if!
intests/deflate-if.rs
.tests/with-compress.rs
.tests/syntax.rs
.Misc
examples/flate.rs
. Especially this was useful for testing the actual binary with decompilers, for me, but should also be useful for people seek to use this crate.