[Rcpp-devel] R_PreserveObject and R_ReleaseObject

[Simon Dirmeier]

Dear all,

I have a question about /R_PreserveObject /and/R_ReleaseObject
/and//thought this would be good place to post it.
I apologize if the question is not suitable or naive.

Suppose I have something like the class below for which I use
Rcpp-modules to extend it to R (code not shown).
I use "insert" to add key-value pair to a map and "remove" to remove an
entry.

I am not having memory issues or so, but the solution seems wrong to me
and I am concerned that it causes problems.
Could you please give me some advice if this implementation is wrong or
unefficient?

Thank you in advance.
Best,
Simon

template<typename T>
class map
{
public:
    map() = default;

    size_t size()
    {
        return map_.size();
    }

    void insert(T& t, SEXP u)
    {
        SEXP s = Rf_duplicate(VECTOR_ELT(u, i));
        R_PreserveObject(s);

        map_.insert(std::pair<T, SEXP>(t[i], s));
    }

    void remove(T& t)
    {
            auto iter = map_.equal_range(t);
            for (auto it = iter.first; it != iter.second; ++it)
            {
                R_ReleaseObject(it->second);
            }
            map_.erase(t);
    }

private:
    std::unordered_map<T, SEXP> map_;
};

[Dirk Eddelbuettel]

On 16 August 2018 at 10:10, Simon Dirmeier wrote:
| Dear all,
|
| I have a question about /R_PreserveObject /and/R_ReleaseObject
| /and//thought this would be good place to post it.
| I apologize if the question is not suitable or naive.

This is the place.

| Suppose I have something like the class below for which I use
| Rcpp-modules to extend it to R (code not shown).
| I use "insert" to add key-value pair to a map and "remove" to remove an
| entry.
|
| I am not having memory issues or so, but the solution seems wrong to me

I would agree. You are using a Standard Template Type; these manage their memory.

The two functions you found are R internals for setting reference counters on
SEXP objects. Think of them as 'free floating' objects -- the ones we receive
from R or return to R. Yours is not one of those, and that it may contain
SEXP does not matter as you do not seem to return these SEXPs to R. If you
did then you would have do that.

And even then you should not need to as Rcpp maps every R with one of our
classes. These have now about 10 years of testing behind and _work_. So why
not take advantage of that?

Of course, this is C so if you know what you are doing, you can whatever you
want. But then you would not need to ask this question :)

Dirk

| and I am concerned that it causes problems.
| Could you please give me some advice if this implementation is wrong or
| unefficient?
|
| Thank you in advance.
| Best,
| Simon
|
| template<typename T>
| class map
| {
| public:
|     map() = default;
|
|     size_t size()
|     {
|         return map_.size();
|     }
|
|     void insert(T& t, SEXP u)
|     {
|         SEXP s = Rf_duplicate(VECTOR_ELT(u, i));
|         R_PreserveObject(s);
|
|         map_.insert(std::pair<T, SEXP>(t[i], s));
|     }
|
|     void remove(T& t)
|     {
|             auto iter = map_.equal_range(t);
|             for (auto it = iter.first; it != iter.second; ++it)
|             {
|                 R_ReleaseObject(it->second);
|             }
|             map_.erase(t);
|     }
|
| private:
|     std::unordered_map<T, SEXP> map_;
| };
|
|
|
| _______________________________________________
| Rcpp-devel mailing list
| Rcpp-***@lists.r-forge.r-project.org
| https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel

[Simon Dirmeier]

Thanks, Dirk. I am just trying to understand how R works under the
surface :/.
Correct me where I am wrong please: if I have another method called
/get/, then I would need to preserve the objects like I did until it is
removed from the map?

template<typename T>
class map
{
public:

   // as before

   SEXP get(T& t)
    {
        return Rcpp::wrap(map_[t]);
    }

private:
    std::unordered_map<T, SEXP> map_;
};

Best,
Simon

Post by Dirk Eddelbuettel
| Dear all,
|
| I have a question about /R_PreserveObject /and/R_ReleaseObject
| /and//thought this would be good place to post it.
| I apologize if the question is not suitable or naive.
This is the place.
| Suppose I have something like the class below for which I use
| Rcpp-modules to extend it to R (code not shown).
| I use "insert" to add key-value pair to a map and "remove" to remove an
| entry.
|
| I am not having memory issues or so, but the solution seems wrong to me
I would agree. You are using a Standard Template Type; these manage their memory.
The two functions you found are R internals for setting reference counters on
SEXP objects. Think of them as 'free floating' objects -- the ones we receive
from R or return to R. Yours is not one of those, and that it may contain
SEXP does not matter as you do not seem to return these SEXPs to R. If you
did then you would have do that.
And even then you should not need to as Rcpp maps every R with one of our
classes. These have now about 10 years of testing behind and _work_. So why
not take advantage of that?
Of course, this is C so if you know what you are doing, you can whatever you
want. But then you would not need to ask this question :)
Dirk
| and I am concerned that it causes problems.
| Could you please give me some advice if this implementation is wrong or
| unefficient?
|
| Thank you in advance.
| Best,
| Simon
|
| template<typename T>
| class map
| {
|     map() = default;
|
|     size_t size()
|     {
|         return map_.size();
|     }
|
|     void insert(T& t, SEXP u)
|     {
|         SEXP s = Rf_duplicate(VECTOR_ELT(u, i));
|         R_PreserveObject(s);
|
|         map_.insert(std::pair<T, SEXP>(t[i], s));
|     }
|
|     void remove(T& t)
|     {
|             auto iter = map_.equal_range(t);
|             for (auto it = iter.first; it != iter.second; ++it)
|             {
|                 R_ReleaseObject(it->second);
|             }
|             map_.erase(t);
|     }
|
|     std::unordered_map<T, SEXP> map_;
| };
|
|
|
| _______________________________________________
| Rcpp-devel mailing list
| https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel

[Dirk Eddelbuettel]

On 16 August 2018 at 15:20, Simon Dirmeier wrote:
| Thanks, Dirk. I am just trying to understand how R works under the
| surface :/.
| Correct me where I am wrong please: if I have another method called
| /get/, then I would need to preserve the objects like I did until it is
| removed from the map?
|
| template<typename T>
| class map
| {
| public:
|
|    // as before
|
|    SEXP get(T& t)
|     {
|         return Rcpp::wrap(map_[t]);
|     }
|
| private:
|     std::unordered_map<T, SEXP> map_;
| };

You don't need to do anything else because of Rcpp. Using `wrap()` to convert
"whatever" (as long it is supported / has a converter) is the way to go, as
our documentation states in several places and as many other packages do.

Dirk

| Best,
| Simon
|
| Am 16.08.18 um 14:00 schrieb Dirk Eddelbuettel:
| > On 16 August 2018 at 10:10, Simon Dirmeier wrote:
| > | Dear all,
| > |
| > | I have a question about /R_PreserveObject /and/R_ReleaseObject
| > | /and//thought this would be good place to post it.
| > | I apologize if the question is not suitable or naive.
| >
| > This is the place.
| >
| > | Suppose I have something like the class below for which I use
| > | Rcpp-modules to extend it to R (code not shown).
| > | I use "insert" to add key-value pair to a map and "remove" to remove an
| > | entry.
| > |
| > | I am not having memory issues or so, but the solution seems wrong to me
| >
| > I would agree. You are using a Standard Template Type; these manage their memory.
| >
| > The two functions you found are R internals for setting reference counters on
| > SEXP objects. Think of them as 'free floating' objects -- the ones we receive
| > from R or return to R. Yours is not one of those, and that it may contain
| > SEXP does not matter as you do not seem to return these SEXPs to R. If you
| > did then you would have do that.
| >
| > And even then you should not need to as Rcpp maps every R with one of our
| > classes. These have now about 10 years of testing behind and _work_. So why
| > not take advantage of that?
| >
| > Of course, this is C so if you know what you are doing, you can whatever you
| > want. But then you would not need to ask this question :)
| >
| > Dirk
| >
| > | and I am concerned that it causes problems.
| > | Could you please give me some advice if this implementation is wrong or
| > | unefficient?
| > |
| > | Thank you in advance.
| > | Best,
| > | Simon
| > |
| > | template<typename T>
| > | class map
| > | {
| > | public:
| > |     map() = default;
| > |
| > |     size_t size()
| > |     {
| > |         return map_.size();
| > |     }
| > |
| > |     void insert(T& t, SEXP u)
| > |     {
| > |         SEXP s = Rf_duplicate(VECTOR_ELT(u, i));
| > |         R_PreserveObject(s);
| > |
| > |         map_.insert(std::pair<T, SEXP>(t[i], s));
| > |     }
| > |
| > |     void remove(T& t)
| > |     {
| > |             auto iter = map_.equal_range(t);
| > |             for (auto it = iter.first; it != iter.second; ++it)
| > |             {
| > |                 R_ReleaseObject(it->second);
| > |             }
| > |             map_.erase(t);
| > |     }
| > |
| > | private:
| > |     std::unordered_map<T, SEXP> map_;
| > | };
| > |
| > |
| > |
| > | _______________________________________________
| > | Rcpp-devel mailing list
| > | Rcpp-***@lists.r-forge.r-project.org
| > | https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel
| >
|
| _______________________________________________
| Rcpp-devel mailing list
| Rcpp-***@lists.r-forge.r-project.org
| https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel

[Dirk Eddelbuettel]

On 16 August 2018 at 15:20, Simon Dirmeier wrote:
| Thanks, Dirk. I am just trying to understand how R works under the
| surface :/.

That is a noble and good goal.

You may get there faster (and/or appreciate sooner just how helpful Rcpp is)
if you did in plain C(++) and the C API of R only leaving Rcpp out. And which
is entirely doable. We "only" have a usage penetration of around 40+ per cent
of CRAN packages with src/ directories so it is of course entirely possibly
to do without, and a good learning exercise if you are into this type of
question.

Dirk

[Kevin Ushey]

I think the main problem is that it's easy to use your class incorrectly.
For example:

- What happens if your map is destructed? The SEXPs it is handling are
effectively leaked.
- What if you copy your map? Since SEXPs are pointers, you now have two
maps containing the same pointers, and if you were to attempt to remove a
SEXP from both maps problems would occur.

Rather than attempting to manage the lifetime of these R objects in your
map's methods, it's better to have wrapper classes that manage the lifetime
of a single SEXP -- ie, what Rcpp does with its classes.

You might consider just using e.g. std::unordered_map<T, Rcpp::RObject> if
you need to map some arbitrary types to R objects.

Best,
Kevin

Post by Simon Dirmeier
Dear all,
I have a question about *R_PreserveObject *and* R_ReleaseObject *and thought
this would be good place to post it.
I apologize if the question is not suitable or naive.
Suppose I have something like the class below for which I use Rcpp-modules
to extend it to R (code not shown).
I use "insert" to add key-value pair to a map and "remove" to remove an
entry.
I am not having memory issues or so, but the solution seems wrong to me
and I am concerned that it causes problems.
Could you please give me some advice if this implementation is wrong or
unefficient?
Thank you in advance.
Best,
Simon
template<typename T>
class map
{
map() = default;
size_t size()
{
return map_.size();
}
void insert(T& t, SEXP u)
{
SEXP s = Rf_duplicate(VECTOR_ELT(u, i));
R_PreserveObject(s);
map_.insert(std::pair<T, SEXP>(t[i], s));
}
void remove(T& t)
{
auto iter = map_.equal_range(t);
for (auto it = iter.first; it != iter.second; ++it)
{
R_ReleaseObject(it->second);
}
map_.erase(t);
}
std::unordered_map<T, SEXP> map_;
};
_______________________________________________
Rcpp-devel mailing list
https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel

[Dirk Eddelbuettel]

On 16 August 2018 at 08:43, Kevin Ushey wrote:
| I think the main problem is that it's easy to use your class incorrectly.
| For example:
|
| - What happens if your map is destructed? The SEXPs it is handling are
| effectively leaked.
| - What if you copy your map? Since SEXPs are pointers, you now have two
| maps containing the same pointers, and if you were to attempt to remove a
| SEXP from both maps problems would occur.
|
| Rather than attempting to manage the lifetime of these R objects in your
| map's methods, it's better to have wrapper classes that manage the lifetime
| of a single SEXP -- ie, what Rcpp does with its classes.
|
| You might consider just using e.g. std::unordered_map<T, Rcpp::RObject> if
| you need to map some arbitrary types to R objects.

+1, or "what he said" which I generally have on auto-loop for Kevin anyway ...

Dirk

[Simon Dirmeier]

Thanks Dirk and Kevin. That cleared a lot of things up.

Best,
Simon

Post by Dirk Eddelbuettel
| I think the main problem is that it's easy to use your class incorrectly.
|
| - What happens if your map is destructed? The SEXPs it is handling are
| effectively leaked.
| - What if you copy your map? Since SEXPs are pointers, you now have two
| maps containing the same pointers, and if you were to attempt to remove a
| SEXP from both maps problems would occur.
|
| Rather than attempting to manage the lifetime of these R objects in your
| map's methods, it's better to have wrapper classes that manage the lifetime
| of a single SEXP -- ie, what Rcpp does with its classes.
|
| You might consider just using e.g. std::unordered_map<T, Rcpp::RObject> if
| you need to map some arbitrary types to R objects.
+1, or "what he said" which I generally have on auto-loop for Kevin anyway ...
Dirk