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

[RFC] Adding const versions of Objects. #1156

Open
mhermier opened this issue Mar 16, 2023 · 9 comments
Open

[RFC] Adding const versions of Objects. #1156

mhermier opened this issue Mar 16, 2023 · 9 comments

Comments

@mhermier
Copy link
Contributor

mhermier commented Mar 16, 2023

Hi,
For reasons, some const versions of objects are required from time to time. I can think of at least 3 strategies:

  • Duplicating the tree with const versions of some class. Tedious, can be out of sync, introduce potential hierarchical and naming issues...
  • Adding a ``Object.isConstattribute withObject.constify()` (or `isSealed` with `seal()`). It would forbid write access inside the VM of `ObjInstance`s (aka all the regular objects). It can also be used in `Class` to forbid subclassing after the hierarchy is created(I remember proposing something similar with `final` but that ones make somehow more sense). It would have the drawback to be best effort in other cases.
  • Do nothing and trust the users.

edit: Removed the part of using it for controlling subclassing. It would make more sense if it was controlling mutation of static variables. But due to implementation details, it is not possible without heavy change in the Class implementation.

@PureFox48
Copy link
Contributor

Well, of the built-in classes, several are immutable already and, to my mind, the only other ones where it might be useful to have const versions are List and Map.

TupleConst which you've just proposed in #1151 would give us a const List and I don't think a const Map is worth the effort in core - if you really need it, it's easy to write your own.

So I think I'd be very much in the 'do nothing' camp here and leave it to library writers or other users to create const versions of their classes if they think they'd be useful.

Sealing classes is, of course, a different matter to immutability and most of the built-in classes are implicitly sealed for good reason. In general, though, I'd be against sealing user defined classes as my experience with other languages which allow this is that it can be a nuisance in practice and people will try to find ways around it.

I didn't follow your point about controlling mutation of static variables as they aren't inheritable (at present anyway) and you can control access to them by just having a getter but no setter.

@mhermier
Copy link
Contributor Author

While I understand your points, a TupleConst while providing almost similar functions to a const List differs from it in 2 critical points (I conserve the example here but I consider it a good representative for the general case):

  • it is not a List, so myConstList is List will return false and can be a source of surprise and code bloat.
  • it has to be kept in sync with the const List or at least with const Tuple, requiring code duplication
    By constify()ing an object, we can solve these issues.

As of today, the static variables (__foo) are stored within the module data, and not in the class. It means the isConstant flag would have no effect, since the access to these variable are translated to direct access to the module variables stack.

@PureFox48
Copy link
Contributor

I'm just throwing ideas around here but one answer to this would be to have a ConstList and interpose that in the inheritance chain from Sequence. So both List and Tuple would inherit from ConstList and thereby avoid a fair bit of code duplication.

@mhermier
Copy link
Contributor Author

If interface become a concrete thing, most functionality could go in there, but that would not solve hierarchy either.
Will look if there is precedence somewhere in one of the languages...

@mhermier
Copy link
Contributor Author

One solution from some java developer: using attributes is to create an ImmutableProxy automagically. It makes the process more straight forward.

@PureFox48
Copy link
Contributor

That's a possibility though currently attributes in Wren are just user-defined so we'd need to extend that to allow built-in ones.

Javascript has an Object.freeze method but I don't think that would be practicable in Wren. Every object would need to have a boolean isFrozen field and methods which attempted to mutate that object would need to check that field to see if it were allowable.

@mhermier
Copy link
Contributor Author

Seems there is also prior art in Pharo Smalltalk with isReadOnlyObject:.

In practice, it should only need to be checked for primitives/foreign, and for static methods (because of implementation details). We can automatically intercept writes to instances in STORE_FIELD_THIS STORE_FIELD opcodes. I'll give it a try using java naming (unless there is a better name). I want to see how much it does impact performances, out of curriosity.

@mhermier
Copy link
Contributor Author

Made a basic implementation. I still have to add test to ensure it works properly before I can publish something more seriously. Thought the initial penalty is quite "low".

api_call - wren                .......... 0.10s 0.0021  93.99% relative to baseline
api_foreign_method - wren      .......... 0.58s 0.0078  99.56% relative to baseline
binary_trees - wren            .......... 0.34s 0.0047  94.96% relative to baseline
binary_trees_gc - wren         .......... 1.33s 0.0079  96.51% relative to baseline
delta_blue - wren              .......... 0.29s 0.0039 101.85% relative to baseline
fib - wren                     .......... 0.40s 0.0123  99.48% relative to baseline
fibers - wren                  .......... 0.06s 0.0028  98.51% relative to baseline
for - wren                     .......... 0.16s 0.0025 100.39% relative to baseline
method_call - wren             .......... 0.22s 0.0048 108.67% relative to baseline
map_numeric - wren             .......... 1.28s 0.0133 100.04% relative to baseline
map_string - wren              .......... 0.14s 0.0012  99.24% relative to baseline
string_equals - wren           .......... 0.27s 0.0048  99.24% relative to baseline

That said, I made a quick check about likely and unlikely (__builtin_expect) branch prediction. We really need to investigate for inclusion. After adding some in the hot simulation loop on top of the previous change:

api_call - wren                .......... 0.10s 0.0041 100.76% relative to baseline
api_foreign_method - wren      .......... 0.41s 0.0017 140.66% relative to baseline
binary_trees - wren            .......... 0.30s 0.0037 106.91% relative to baseline
binary_trees_gc - wren         .......... 1.30s 0.0043  98.56% relative to baseline
delta_blue - wren              .......... 0.29s 0.0028 102.53% relative to baseline
fib - wren                     .......... 0.38s 0.0021 106.60% relative to baseline
fibers - wren                  .......... 0.06s 0.0037 100.27% relative to baseline
for - wren                     .......... 0.16s 0.0039 101.67% relative to baseline
method_call - wren             .......... 0.21s 0.0052 114.42% relative to baseline
map_numeric - wren             .......... 1.28s 0.0217  99.86% relative to baseline
map_string - wren              .......... 0.14s 0.0014 100.98% relative to baseline
string_equals - wren           .......... 0.26s 0.0077 100.42% relative to baseline

@PureFox48
Copy link
Contributor

Better than I expected. Might be a viable approach after all :)

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