[R] Ensure `ProxyDMatrix` creation keeps data until next iteration #11092

david-cortes · 2024-12-11T17:38:39Z

This PR modifies the logic of the ProxyDMatrix loop to ensure that whatever data it gets fed will be kept alive until the next iteration.

trivialfis · 2024-12-11T17:42:30Z

R-package/src/xgboost_R.cc

@@ -682,6 +682,7 @@ XGB_DLL SEXP XGProxyDMatrixCreate_R() {

 XGB_DLL SEXP XGProxyDMatrixSetDataDense_R(SEXP handle, SEXP R_mat) {
  R_API_BEGIN();
+  R_SetExternalPtrProtected(handle, R_mat);


Hmm, so when is the data released?

My understanding is that the external pointer object has only one slot for a protected SEXP object. So a call to this function would make the previous object in that slot become unprotected:
https://github.com/wch/r-source/blob/872770ededfc26429adca09fab0803e1b78b6eac/src/main/memory.c#L3855
https://github.com/wch/r-source/blob/872770ededfc26429adca09fab0803e1b78b6eac/src/main/memory.c#L1292

But I'm not 100% sure about it.

Going through C macros is not my strong suit ...

Do you think it's difficult to store the data in the ProxyDMatrixWrapper you wrote in xgboost_R.cc? Currently, it's not used in the R code, but we can make the R proxy dmatrix creator return the wrapper instead of the real proxy dmatrix handler.

In Python, I store the temporary data in the iterator, but anywhere is fine.

Going through the code again, it looks like both xgb.ExtMemDMatrix and xgb.QuantileDMatrix.from_iterator will end up calling one of these 3 functions in the end, so please ignore the latter comment.

Do you think it's difficult to store the data in the ProxyDMatrixWrapper you wrote in xgboost_R.cc?

I think it could be quite tricky, because the public R C API doesn't allow protecting/unprotecting individual objects (unless by the logic of putting an unprotected object inside a protected container like list/externalptr) - it only offers UNPROTECT(<last N>).

It'd have to use a list object and be constantly assigning the same element to it in order to achieve the same effect, which wouldn't have any advantage compared to the mechanism in this PR of assigning to the "protected" slot of the externalptr, assuming that it works the way we expect it to.

assuming that it works the way we expect it to.

That's internal to R right? Even if it works today, it can break at the next release and we don't have a way to test that.

assuming that it works the way we expect it to.

That's internal to R right? Even if it works today, it can break at the next release and we don't have a way to test that.

No, that's part of the public C API of R, which has pretty strong guarantees. Internal API calls in package code would disqualify packages from being hosted at CRAN.

And either way, it looks like it should end up using the macro from the earlier link that decreases the reference count, unless you manually happen to define internal-use variables before including the R header:
https://github.com/wch/r-source/blob/872770ededfc26429adca09fab0803e1b78b6eac/src/include/Rinternals.h#L191

No, that's part of the public C API of R

hmm, I meant the "external pointer object has only one slot for a protected SEXP object. So a call to this function would make the previous object in that slot become unprotected" behavior. I think it can change without notice since it's not documented.

No, that's part of the public C API of R

hmm, I meant the "external pointer object has only one slot for a protected SEXP object. So a call to this function would make the previous object in that slot become unprotected" behavior. I think it can change without notice since it's not documented.

I highly doubt that'd the case, because that's how it works for other objects like lists too, even if the behavior is not documented for all classes.

And even for undocumented behavior, the R team is constantly testing these sorts of things with third-package packages and only makes breaking changes after notifying affected packages, with a very long notice period given to make the change and announcements in the release notes for changes in behavior.

got it, thank you for sharing.

david-cortes · 2024-12-11T17:51:00Z

Looks like xgb.ExtMemDMatrix and xgb.QuantileDMatrix.from_iterator will also need similar protection mechanisms.

david-cortes · 2024-12-11T18:24:45Z

@trivialfis Just to make sure: in this PR, the data is (was, changed with the last commit) being held right up until the next call to XGProxyDMatrixSetData*.

But it gets unprotected before the next call with the next batch of data.

Is that the correct behavior or should it be the other way around? i.e. old batch kept alive until current batch setdata finished.

trivialfis · 2024-12-11T18:43:45Z

Is that the correct behavior or should it be the other way around? i.e. old batch kept alive until current batch setdata finished.

The previous batch can be freed once the next or the reset is called. The events happen in the following order:

iter->next (in R)
generate temp data
set_data called inside the next , exit the next call
xgboost consumes the data in C
iter->next (in R)
generate temp data
set_data called inside the next, exit next call.
xgboost consumes the data in C
...

The free for data generated by 2 can happen between 5 and 8. If freed before 6, then we can save some memory as there's only one batch of temporary data can be in the memory. The saved memory doesn't matter much in most cases.

david-cortes · 2024-12-11T18:49:31Z

Is that the correct behavior or should it be the other way around? i.e. old batch kept alive until current batch setdata finished.

The previous batch can be freed once the next or the reset is called. The events happen in the following order:

iter->next (in R)

generate temp data

set_data called inside the next , exit the next call

xgboost consumes the data in C

iter->next (in R)

generate temp data

set_data called inside the next, exit next call.

xgboost consumes the data in C
...

The free for data generated by 2 can happen between 5 and 8. If freed before 6, then we can save some memory as there's only one batch of temporary data can be in the memory. The saved memory doesn't matter much in most cases.

But when is (4) finalized? In this case, the only interactions with the DMatrix object are through setdata. Or am I missing some additional call?

trivialfis · 2024-12-11T18:50:13Z

But when is (4) finalized?

When 5 happens.

david-cortes · 2024-12-11T18:53:54Z

But when is (4) finalized?

When 5 happens.

Meaning: inside the loop in *FromCallback before calling XGDMatrixCallbackNext(DataIterHandle) ?

trivialfis · 2024-12-11T18:54:20Z

the only interactions with the DMatrix object are through setdata. Or am I missing some additional call?

No missing call.

In the previous comment, both the setdata and generation of temp data happen INSIDE the next call:

function iter.next() {
   # free here
    xgb.ProxyDMatrix(  # The R function defined in line 555 in xgb.DMatrix.R
        temp = process_df()
        set_data(temp)
    )
    # free here works as well, but we can have two temp data in the memory.
}

Inside xgboost:

while iter.next():
    consume_data_in_proxy_dmatrix

david-cortes · 2024-12-11T19:01:30Z

I'm still not getting when exactly would the DMatrix be finished with the next batch, but I've added some calls to unprotect the previous batch right before the call to the R-side 'next'. It should in theory allow freeing the earlier batch before the next one is allocated in memory when using external memory.

Please take a look to see if that's the correct place.

trivialfis · 2024-12-11T19:04:07Z

I'm still not getting when exactly would the DMatrix be finished with the next batch

may I ask where is the confusion? Apologies for my poor explanation. If a new batch is requested by XGBoost, then the old batch is finished, and XGBoost requests a new batch by calling the next function. Not entirely sure where it got complicated.

david-cortes · 2024-12-11T19:08:51Z

I'm still not getting when exactly would the DMatrix be finished with the next batch

may I ask where is the confusion? Apologies for my poor explanation. If a new batch is requested by XGBoost, then the old batch is finished, and XGBoost requests a new batch by calling the next function. Not entirely sure where it got complicated.

Got it now. So then the current logic as of the last commit should work in such a way that it doesn't require two batches to co-exist in memory.

But wouldn't it be more logical to make it so that the data is consumed during the call to setdata and not referenced after that call, considering that there aren't any further user functions running between the two?

trivialfis · 2024-12-11T19:09:08Z

The previous batch can be freed inside anywhere of this function:

xgboost/R-package/R/xgb.DMatrix.R

Line 557 in 376009c

lst <- data_iterator$f_next(data_iterator$env)

preferably before:

xgboost/R-package/R/xgb.DMatrix.R

Line 569 in 376009c

tmp <- .process.df.for.dmatrix(lst$data, lst$feature_types)

since we will have two temporary batches (old and new tmp) in memory if we free after this line. But it's fine.

trivialfis · 2024-12-11T19:15:17Z

But wouldn't it be more logical to make it so that the data is consumed during the call to setdata

It couldn't, the proxy dmatrix is just a proxy to the user input as the name suggests, and doesn't do anything other than hold the reference to the data. Consider it a "data view" that XGBoost understands, instead of a container of the data. We have to create something that wraps the high-level user input for the C++ code to consume, the proxy dmatrix is the wrapper. I should probably call it DataReference/DataWrapper or something like that instead of ProxyDMatrix.

david-cortes · 2024-12-11T19:15:30Z

The previous batch can be freed inside anywhere of this function:

xgboost/R-package/R/xgb.DMatrix.R

Line 557 in 376009c

lst <- data_iterator$f_next(data_iterator$env)

preferably before:

xgboost/R-package/R/xgb.DMatrix.R

Line 569 in 376009c

tmp <- .process.df.for.dmatrix(lst$data, lst$feature_types)

since we will have two temporary batches (old and new tmp) in memory if we free after this line. But it's fine.

Then the current placement should be fine, since this function:

xgboost/R-package/R/xgb.DMatrix.R

Line 556 in 376009c

xgb.ProxyDMatrix <- function(proxy_handle, data_iterator) {

Is only called from the C++ side here:

xgboost/R-package/src/xgboost_R.cc

Line 747 in 376009c

SEXP R_res = Rf_protect(

.. through wrappers like these:

xgboost/R-package/R/xgb.DMatrix.R

Line 365 in 376009c

iterator_next <- function() {

trivialfis

Thank you for the fix! Looks great.

david-cortes · 2024-12-14T16:54:28Z

Looks like this didn't work out correctly. Now it ends up with a null pointer derefence somewhere.

trivialfis · 2024-12-15T07:45:56Z

Maybe we lower the data transformation into C and store the temporary data in the proxy wrapper?

ensure data is kept alive until next set or reset

678860e

trivialfis reviewed Dec 11, 2024

View reviewed changes

unprotect only after setting next batch

f0de83e

unprotect data before generating the next batch

34110fe

typo

98bdc7f

trivialfis approved these changes Dec 11, 2024

View reviewed changes

trivialfis merged commit 3162e0d into dmlc:master Dec 11, 2024
57 of 58 checks passed

trivialfis mentioned this pull request Dec 13, 2024

Auto encoding for categorical data during inference. #11088

Open

6 tasks

david-cortes mentioned this pull request Dec 15, 2024

[R] Move gc data protection to R side #11104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R] Ensure `ProxyDMatrix` creation keeps data until next iteration #11092

[R] Ensure `ProxyDMatrix` creation keeps data until next iteration #11092

david-cortes commented Dec 11, 2024

trivialfis Dec 11, 2024

david-cortes Dec 11, 2024

trivialfis Dec 11, 2024

trivialfis Dec 11, 2024

david-cortes Dec 11, 2024

trivialfis Dec 11, 2024

david-cortes Dec 11, 2024

trivialfis Dec 11, 2024 •

edited

Loading

david-cortes Dec 11, 2024

trivialfis Dec 11, 2024

david-cortes commented Dec 11, 2024

david-cortes commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 •

edited

Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 •

edited

Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

david-cortes commented Dec 11, 2024

trivialfis left a comment

david-cortes commented Dec 14, 2024

trivialfis commented Dec 15, 2024 •

edited

Loading

[R] Ensure ProxyDMatrix creation keeps data until next iteration #11092

[R] Ensure ProxyDMatrix creation keeps data until next iteration #11092

Conversation

david-cortes commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trivialfis Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-cortes commented Dec 11, 2024

david-cortes commented Dec 11, 2024 • edited Loading

trivialfis commented Dec 11, 2024 • edited Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 • edited Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 • edited Loading

david-cortes commented Dec 11, 2024

trivialfis commented Dec 11, 2024 • edited Loading

trivialfis commented Dec 11, 2024 • edited Loading

david-cortes commented Dec 11, 2024

trivialfis left a comment

Choose a reason for hiding this comment

david-cortes commented Dec 14, 2024

trivialfis commented Dec 15, 2024 • edited Loading

[R] Ensure `ProxyDMatrix` creation keeps data until next iteration #11092

[R] Ensure `ProxyDMatrix` creation keeps data until next iteration #11092

trivialfis Dec 11, 2024 •

edited

Loading

david-cortes commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 11, 2024 •

edited

Loading

trivialfis commented Dec 15, 2024 •

edited

Loading