Switch log signature to str from bytes #366
Conversation
austinabell
requested review from
chadoh,
evgenykuzyakov and
mikedotexe
as code owners
|
Tested the different APIs with a modified greeter to just have the logs as follows: log!("Saving greeting '{}' for account '{}'", message, account_id,);
log!("Some static log");
log!(message.as_str());and even in that case that I assumed it would optimize away, it wasn't: ~/dev/git/nea/log/con/tar/was/release $ stat -c '%n %s' greeter.wasm
greeter.wasm 120783
~/dev/git/nea/log/con/tar/was/release $ stat -c '%n %s' greeter.wasm
greeter.wasm 119963So I decided to switch to the non-generic function signature because the API will be basically the same when using the Edit: I also switched the single arg log macro to be of expr type, because the tt only allowed a static string, and I don't see why it can't also take a variable/expression if someone wanted. |
But the |
Do we have our backwards compatibility policy for SDK documented anywhere? Taking a bird's eye view, the current public API of SDK feels like it has a lot of rough edges from Rust API design point of view, and it seems like long term smoothing those out would be beneficial. However, it's unclear to me at least what is the best path towards that. I can see several options for this particular change, and for this class of changes:
Neither of them is inherently better than the other (even the last one can be beneficial in the exceptional circumstances), but it's unclear which one we use. |
|
I've updated the code to keep the existing This is not completely backwards compatible because the macro is switched to use the new function, but I can't think of a case where this would actually be hit. Everything that worked before should still work, and this will be released under a major, breaking release anyway. |
| @@ -506,10 +506,20 @@ pub fn panic(message: &[u8]) -> ! { | |||
| unsafe { sys::panic_utf8(message.len() as _, message.as_ptr() as _) } | |||
| unreachable!() | |||
| } | |||
| /// Logs the string message message. This message is stored on chain. | |||
| pub fn log_str(message: &str) { | |||
There was a problem hiding this comment.
Should it be accepting anything that implements ToString trait? WDYT @matklad ?
Btw, is this method going to support no-std?
There was a problem hiding this comment.
I don't see a huge benefit in supporting this because it would increase code size for the monomorphization of the generic and for more general usage, log! macro is used. I originally made it generic based on AsRef<str> but since it added code size (tested and an example given above) the macro calls as_ref and this base call remains as a concrete type. Having it be ToString would force an unnecessary allocation that I'm not sure can be optimized away by the compiler.
As for no_std, yes it is no_std compatible and has the same memory layout as bytes. It's currently used as such in the no_std experimental SDK I've been playing with.
There was a problem hiding this comment.
No, ToString would force allocation, and won't be enough to make a nice API. For the latter, we need a format-style macro.
I've decided to go with the generic. Alternative being takingAsRef<str>to start as it makes for a cleaner API, but I'm unsure if the code duplication from the generic gets optimized away, so I will try to compare now and will leave this PR in draft state until I explore some more&stras the param and just calling.as_ref()in the macro call, which might be best to make sure to minimize the code output size in all potential cases.I asked about this a while ago in discord, but I'll assume #362 means the breaking change is acceptable. Is there any downstream work connected with the master branch that I should update/make sure this doesn't break?
Closes #362