Re: [Xen-devel] [PATCH 1/1] tools: reduce copies b/w ocaml Strings and Bytes

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Apr 05, 2018 at 05:16:27PM +0100, Christian Lindig wrote:
> I think this is a good patch as it reduces the amount of copying. I believe
> it is safe as it is. There is one place where I am a little hesitant:
> 
> @@ -291,7 +291,9 @@ let access_logging ~con ~tid ?(data="") ~level
> access_type =
> 
>                               let date = string_of_date() in
>                               let tid = string_of_tid ~con tid in
>                               let access_type = string_of_access_type 
> access_type in
> -                             let data = sanitize_data (Bytes.of_string data) 
> in
> +                             (* we can use unsafe_of_string here as the 
> sanitize_data function
> +                                immediately makes a copy of the data and 
> operates on that. *)
> +                             let data = sanitize_data 
> (Bytes.unsafe_of_string data) in
>                               let prefix = prefix !access_log_destination 
> date in
>                               let msg = Printf.sprintf "%s %s %s %s" prefix 
> tid access_type data in
>                               logger.write ~level msg)
> 
> This relies on the implementation of sanitize_data() and somebody could
> change it in the future
> and invalidate the assumption being made here. However, this is the only
> call site and the
> function is defined above. Anybody making changes in the context of
> String/Byte conversion
> come across the comment here.
> 
> So: I'm happy to take the patch as it is.
> 
> On 05/04/18 11:40, Marcello Seri wrote:
> > When xenstore was ported to the new safe-string interface, it mostly
> > happened by making copyies of string into bytes and back.  The ideal
> > fix would be to rewrite all of the relevant interfaces to be uniformly
> > using bytes, but in the meanwhile we can improve the code by using unsafe
> > conversion functions (see
> >   
> > https://caml.inria.fr/pub/docs/manual-ocaml/libref/Bytes.html#3_Unsafeconversionsforadvancedusers).
> > 
> > In most cases we own the bytes that we are converting to string, or we
> > immediately make copies that we then mutate, or we use them immutably
> > as payloads for writes. In all these cases it is safe to use the unsafe
> > functions and prevent a copy.
> > 
> > This patch updates the code to use the unsafe conversions where possible.
> > 
> > Signed-off-by: Marcello Seri <marcello.seri@xxxxxxxxxx>
> Reviewed-by: Christian Lindig <christian.lindig@xxxxxxxxxx>

Applied with Juergen's release-ack.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel