Skip to content

Conversation

@tcoopman
Copy link

@tcoopman tcoopman commented Feb 12, 2022

The code is not ready yet, but I wanted to open the PR already because I have some questions. This PR fixes #1238

What should the actual logic be?

  • The current implementation is a lot slower if you add small strings with line breaks vs small string without line breaks (4-5 times slower in some quick tests).
  • What if we have a string that is n * max_length? Do we...
    • add the whole string and flush after that
    • flush n times, every time after max_length
  • If we want to keep the current behavior and flush after line breaks, and we have a string that looks like: foo\n very long .... string after line break", what do we want to do?
    • only flush after \n and add the long string without flushing
    • flush both after \n and do the same as a string with n * max_length
    • ignore the \n and do the same as a string with n * max_length

My proposal would be to do the simplest thing:

  if total_length > max_length : add full string and flush (* even if there are linebreaks in the string *)
  else if we have a line break: flush after the line break
  else just add the string

What do you think?

Should I write a test for this? And how and where should I do it?

I could write some js unit tests, but I don't see any in the code so far, so I'm not sure what to do, just let me know

//Requires: caml_ml_flush,caml_ml_bytes_length
//Requires: caml_create_bytes, caml_blit_bytes, caml_raise_sys_error, caml_ml_channels, caml_string_of_bytes
//Requires: caml_jsbytes_of_string
var MAX_LENGTH = 65536;
Copy link
Author

Choose a reason for hiding this comment

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

where should I place this? At the top of the file and rename it to MAX_BUFFER_LENGTH?

Copy link
Member

Choose a reason for hiding this comment

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

It's only going to be used here. Just put it inside the funciton.

@hhugo
Copy link
Member

hhugo commented Feb 15, 2022

The current implementation is a lot slower if you add small strings with line breaks vs small string without line breaks (4-5 times slower in some quick tests).

The current implementation is only beneficial for stdout and stderr when printing on the console. We could restrict that logic for theses cases only (#1241 allows to identify theses cases)

What if we have a string that is n * max_length? Do we...
add the whole string and flush after that
flush n times, every time after max_length

I'dont have any preference here

@hhugo
Copy link
Member

hhugo commented Mar 3, 2022

Could you please rebase your work on top of #1241 ?

@hhugo
Copy link
Member

hhugo commented May 2, 2022

Merged as part of #1270

@hhugo hhugo closed this May 2, 2022
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.

[BUG] Memory keeps growing with Buffer and js_of_ocaml

2 participants