[PATCH 0/1] Reduce copies between ocaml Strings and Bytes

[PATCH 0/1] Reduce copies between ocaml Strings and Bytes

[Marcello Seri]

The port of the ocaml libraries to the new safe-string interface
introduced some unnecessary copies between ocaml strings and bytes. The
bytes module provides unsafe conversion functions that avoid the copies
and are safe to use when the bytes are used immutably (as in Unix.write
calls) or when the ownership of the bytes is clear. For reference see
https://caml.inria.fr/pub/docs/manual-ocaml/libref/Bytes.html#3_Unsafeconversionsforadvancedusers

This patch replaces the conversion functions with unsafe conversions
when possible.

Marcello Seri (1):
  tools: reduce copies b/w ocaml Strings and Bytes

 tools/ocaml/libs/xb/xb.ml        | 4 +++-
 tools/ocaml/xenstored/logging.ml | 8 +++++---
 tools/ocaml/xenstored/stdext.ml  | 2 +-
 tools/ocaml/xenstored/utils.ml   | 6 +++---
 4 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.14.1


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

[PATCH 1/1] tools: reduce copies b/w ocaml Strings and Bytes

[Marcello Seri]

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@citrix.com>
---
 tools/ocaml/libs/xb/xb.ml        | 4 +++-
 tools/ocaml/xenstored/logging.ml | 8 +++++---
 tools/ocaml/xenstored/stdext.ml  | 2 +-
 tools/ocaml/xenstored/utils.ml   | 6 +++---
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 519842723b..660224f895 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -100,7 +100,9 @@ let write_mmap back con s len =
 
 let write con s len =
 	match con.backend with
-	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
+	(* we can use unsafe_of_string here as the bytes are used immutably
+	   in the Unix.write operation. *)
+	| Fd backfd     -> write_fd backfd con (Bytes.unsafe_of_string s) len
 	| Xenmmap backmmap -> write_mmap backmmap con s len
 
 (* NB: can throw Reconnect *)
diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
index e3c769fb2c..45a2c222e6 100644
--- a/tools/ocaml/xenstored/logging.ml
+++ b/tools/ocaml/xenstored/logging.ml
@@ -64,7 +64,7 @@ let truncate_line nb_chars line =
 		Bytes.blit_string line 0 dst_line 0 (len - 2);
 		Bytes.set dst_line (len-2) '.';
 		Bytes.set dst_line (len-1) '.';
-		Bytes.to_string dst_line
+		Bytes.unsafe_to_string dst_line
 	else line
 
 let log_rotate ref_ch log_file log_nb_files =
@@ -258,7 +258,7 @@ let sanitize_data data =
 		if Bytes.get data i = '\000' then
 			Bytes.set data i ' '
 	done;
-	String.escaped (Bytes.to_string data)
+	String.escaped (Bytes.unsafe_to_string data)
 
 let activate_access_log = ref true
 let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
@@ -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)
diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
index 41411ee535..869fec36f2 100644
--- a/tools/ocaml/xenstored/stdext.ml
+++ b/tools/ocaml/xenstored/stdext.ml
@@ -122,7 +122,7 @@ let pidfile_write filename =
 		let pid = Unix.getpid () in
 		let buf = string_of_int pid ^ "\n" in
 		let len = String.length buf in
-		if Unix.write fd (Bytes.of_string buf) 0 len <> len
+		if Unix.write fd (Bytes.unsafe_of_string buf) 0 len <> len
 		then failwith "pidfile_write failed";
 	)
 	(fun () -> Unix.close fd)
diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
index 5fcb042351..73affb7ea4 100644
--- a/tools/ocaml/xenstored/utils.ml
+++ b/tools/ocaml/xenstored/utils.ml
@@ -52,7 +52,7 @@ let hexify s =
 		Bytes.set hs (i * 2) seq.[0];
 		Bytes.set hs (i * 2 + 1) seq.[1];
 	done;
-	Bytes.to_string hs
+	Bytes.unsafe_to_string hs
 
 let unhexify hs =
 	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
@@ -61,7 +61,7 @@ let unhexify hs =
 	do
 		Bytes.set b i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
 	done;
-	Bytes.to_string b
+	Bytes.unsafe_to_string b
 
 let trim_path path =
 	try
@@ -87,7 +87,7 @@ let read_file_single_integer filename =
 	let buf = Bytes.make 20 (char_of_int 0) in
 	let sz = Unix.read fd buf 0 20 in
 	Unix.close fd;
-	int_of_string (Bytes.to_string (Bytes.sub buf 0 sz))
+	int_of_string (Bytes.sub_string buf 0 sz)
 
 let path_complete path connection_path =
 	if String.get path 0 <> '/' then
-- 
2.14.1


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

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

[Christian Lindig]

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@citrix.com>
Reviewed-by: Christian Lindig <christian.lindig@citrix.com>

> ---
>   tools/ocaml/libs/xb/xb.ml        | 4 +++-
>   tools/ocaml/xenstored/logging.ml | 8 +++++---
>   tools/ocaml/xenstored/stdext.ml  | 2 +-
>   tools/ocaml/xenstored/utils.ml   | 6 +++---
>   4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
> index 519842723b..660224f895 100644
> --- a/tools/ocaml/libs/xb/xb.ml
> +++ b/tools/ocaml/libs/xb/xb.ml
> @@ -100,7 +100,9 @@ let write_mmap back con s len =
>   
>   let write con s len =
>   	match con.backend with
> -	| Fd backfd     -> write_fd backfd con (Bytes.of_string s) len
> +	(* we can use unsafe_of_string here as the bytes are used immutably
> +	   in the Unix.write operation. *)
> +	| Fd backfd     -> write_fd backfd con (Bytes.unsafe_of_string s) len
>   	| Xenmmap backmmap -> write_mmap backmmap con s len
>   
>   (* NB: can throw Reconnect *)
> diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml
> index e3c769fb2c..45a2c222e6 100644
> --- a/tools/ocaml/xenstored/logging.ml
> +++ b/tools/ocaml/xenstored/logging.ml
> @@ -64,7 +64,7 @@ let truncate_line nb_chars line =
>   		Bytes.blit_string line 0 dst_line 0 (len - 2);
>   		Bytes.set dst_line (len-2) '.';
>   		Bytes.set dst_line (len-1) '.';
> -		Bytes.to_string dst_line
> +		Bytes.unsafe_to_string dst_line
>   	else line
>   
>   let log_rotate ref_ch log_file log_nb_files =
> @@ -258,7 +258,7 @@ let sanitize_data data =
>   		if Bytes.get data i = '\000' then
>   			Bytes.set data i ' '
>   	done;
> -	String.escaped (Bytes.to_string data)
> +	String.escaped (Bytes.unsafe_to_string data)
>   
>   let activate_access_log = ref true
>   let access_log_destination = ref (File (Paths.xen_log_dir ^ "/xenstored-access.log"))
> @@ -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)
> diff --git a/tools/ocaml/xenstored/stdext.ml b/tools/ocaml/xenstored/stdext.ml
> index 41411ee535..869fec36f2 100644
> --- a/tools/ocaml/xenstored/stdext.ml
> +++ b/tools/ocaml/xenstored/stdext.ml
> @@ -122,7 +122,7 @@ let pidfile_write filename =
>   		let pid = Unix.getpid () in
>   		let buf = string_of_int pid ^ "\n" in
>   		let len = String.length buf in
> -		if Unix.write fd (Bytes.of_string buf) 0 len <> len
> +		if Unix.write fd (Bytes.unsafe_of_string buf) 0 len <> len
>   		then failwith "pidfile_write failed";
>   	)
>   	(fun () -> Unix.close fd)
> diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml
> index 5fcb042351..73affb7ea4 100644
> --- a/tools/ocaml/xenstored/utils.ml
> +++ b/tools/ocaml/xenstored/utils.ml
> @@ -52,7 +52,7 @@ let hexify s =
>   		Bytes.set hs (i * 2) seq.[0];
>   		Bytes.set hs (i * 2 + 1) seq.[1];
>   	done;
> -	Bytes.to_string hs
> +	Bytes.unsafe_to_string hs
>   
>   let unhexify hs =
>   	let char_of_hexseq seq0 seq1 = Char.chr (int_of_string (sprintf "0x%c%c" seq0 seq1)) in
> @@ -61,7 +61,7 @@ let unhexify hs =
>   	do
>   		Bytes.set b i (char_of_hexseq hs.[i * 2] hs.[i * 2 + 1])
>   	done;
> -	Bytes.to_string b
> +	Bytes.unsafe_to_string b
>   
>   let trim_path path =
>   	try
> @@ -87,7 +87,7 @@ let read_file_single_integer filename =
>   	let buf = Bytes.make 20 (char_of_int 0) in
>   	let sz = Unix.read fd buf 0 20 in
>   	Unix.close fd;
> -	int_of_string (Bytes.to_string (Bytes.sub buf 0 sz))
> +	int_of_string (Bytes.sub_string buf 0 sz)
>   
>   let path_complete path connection_path =
>   	if String.get path 0 <> '/' then


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

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

[Wei Liu]

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@citrix.com>
> Reviewed-by: Christian Lindig <christian.lindig@citrix.com>

Applied with Juergen's release-ack.

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