Skip to content
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

It seems there is a memory leak in constant memory enabled when trying to insert date #453

Closed
videni opened this issue Aug 27, 2024 · 7 comments

Comments

@videni
Copy link

videni commented Aug 27, 2024

Sorry, I don't know much about C. I can't reproduce it with C. I only found the problem in here. I copy the critical code here if you can spot it quickly. The code enabled const memory is not copied here, but it is enabled in my local. The code looks very normal.

PHP_METHOD(vtiful_xls, insertDate)
{
    zval *data = NULL, *format_handle = NULL;
    zend_long row = 0, column = 0;
    zend_string *format = NULL;

    ZEND_PARSE_PARAMETERS_START(3, 5)
            Z_PARAM_LONG(row)
            Z_PARAM_LONG(column)
            Z_PARAM_ZVAL(data)
            Z_PARAM_OPTIONAL
            Z_PARAM_STR_OR_NULL(format)
            Z_PARAM_RESOURCE_OR_NULL(format_handle)
    ZEND_PARSE_PARAMETERS_END();

    ZVAL_COPY(return_value, getThis());

    xls_object *obj = Z_XLS_P(getThis());

    WORKBOOK_NOT_INITIALIZED(obj);
    SHEET_LINE_SET(obj, row);

    if (Z_TYPE_P(data) != IS_LONG) {
        zend_throw_exception(vtiful_exception_ce, "timestamp is long", 160);
        return;
    }

    // Default datetime format
    if (format == NULL || (format != NULL && ZSTR_LEN(format) == 0)) {
        format = zend_string_init(ZEND_STRL("yyyy-mm-dd hh:mm:ss"), 0);
    }

    lxw_datetime datetime = timestamp_to_datetime(data->value.lval);

    if (format_handle != NULL) {
        datetime_writer(&datetime, row, column, format, &obj->write_ptr, zval_get_format(format_handle));
    } else {
        datetime_writer(&datetime, row, column, format, &obj->write_ptr, obj->format_ptr.format);
    }

    // Release default format
    if (ZEND_NUM_ARGS() == 3) {
        zend_string_release(format);
    }
}
/*
 * Write the datetime to the file
 */
void datetime_writer(lxw_datetime *datetime, zend_long row, zend_long columns, zend_string *format, xls_resource_write_t *res, lxw_format *format_handle)
{
    lxw_format *value_format = workbook_add_format(res->workbook);

    if (format_handle != NULL) {
        format_copy(value_format, format_handle);
    }

    format_set_num_format(value_format, ZSTR_VAL(format));
    worksheet_write_datetime(res->worksheet, (lxw_row_t)row, (lxw_col_t)columns, datetime, value_format);
}
@videni videni changed the title it seems there is memory leak in const memory enabled when try to insert date It seems there is a memory leak in constant memory enabled when trying to insert date Aug 27, 2024
@jmcnamara
Copy link
Owner

  • What OS are you using?
  • What hardware is it?
  • On the OS is the /tmp directory mapped to system memory?
  • Are you seeing a "leak" where the application doesn't return memory to the OS after it completes?
  • Or are you seeing a situation where the application just consumes memory as it runs?

If it is the last one and your /tmp is mapped to memory then the application is consuming disk space but that disk space has been mapped to memory.

If this is the case then libxlsxwriter has an option to change the location of the temp files it used:

http://libxlsxwriter.github.io/workbook_8h.html#a7329be90faba9e9ee65f58cb221aa4f1

Check if php-ext-xlsxwriter has the same opinion.

Note, libxlsxwriter has hundreds of tests for memory leaks that run in the CI so it is unlikely that it is an actual leak.

@videni
Copy link
Author

videni commented Aug 27, 2024

@jmcnamara, thansk for the tips, there is no option to change the location of the temp files in the php-ext-xlsxwriter , and the problem it not the tmp files, I run the code on MacOS, Ubuntu , Alpine linux, all have the issue.

I think it can reproduced by this link, you can observe continious growth of memory when constant memory enabled and insert 1 million rows with a datetime. sorry it is PHP code.

@jmcnamara
Copy link
Owner

Ok. Sorry I can't help you other than the advice above.

@videni
Copy link
Author

videni commented Aug 27, 2024

@jmcnamara after some experiments, I found this line caused the memory leak. does the code below remind you something please?

the below are from the datetime_writer method above.

// it might this line. because  I inserted 1 millon datetime object,  the value_format will be created that much, 
// I think somehow, this object not released in const memory. 
 lxw_format *value_format = workbook_add_format(res->workbook);

  if (format_handle != NULL) {
      format_copy(value_format, format_handle);
  }

@jmcnamara
Copy link
Owner

That definitely looks like an issue. It seems to be creating a new format object for each call to datetime_writer(). These objects are cleaned up on exit so it isn't a leak. However, it will cause a memory growth. Especially, if you writing a million cells.

You should contact the author and let them know.

You may be able to work around it by writing a number instead of a date. In Excel a date is a number with a format. You will need to covert the dates yourself but it will avoid this issue (unless the number writer is doing the same thing).

See: https://libxlsxwriter.github.io/working_with_dates.html

@videni
Copy link
Author

videni commented Aug 27, 2024

@jmcnamara I see, thanks a lot. Finally, I understand what the real problem is. The php-ext-xlswriter almost calls the workbook_add_format method for each cell. that issue existed for 2 years, still not fixed. Some design issues might exist , not easy to fix. There might be two ways to solve the problem:

  1. Reuse existing from upper level code.
  2. Or don't create format for each cell, but use the format set by set_column method instead.

except strings, the number has the same issue as datetime, if no format passed for the worksheet_write_string, worksheet_write_number, worksheet_write_datetime, it will use the format set by theset_column method , right?

@videni
Copy link
Author

videni commented Aug 28, 2024

Closed as solved by php-ext-xlswriter, @jmcnamara thanks for help,

@videni videni closed this as completed Aug 28, 2024
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

No branches or pull requests

2 participants