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

New function for maps/regexps #437

Merged
merged 5 commits into from
Feb 13, 2021
Merged

Conversation

ALANVF
Copy link
Contributor

@ALANVF ALANVF commented Jan 9, 2021

So for starters, I fixed the installation issue on Linux that everyone seems to be complaining about.

Secondly, I added 2 (technically 4) functions to the builtin stdlib, which is step 1 of several for a PR that I'm working on for Haxe.
The functions are as follows:

  • hisize/hbsize/hosize: Return the number of entries in the map.
  • regexp_matched_num: Return the number of matched groups from the last time a regexp was matched.

@ncannasse
Copy link
Member

For Makefile: it's actually intended the way it is atm, I don't think you should add paths outside of the current directory for security reasons, as it might load untrusted binaries with privileges without notice.

For Maps : I understand the need to have the size but I don't think we will add it to Haxe, because some platforms will have a very costly size() implementation, and knowing a Map size is in general not very useful.

@ALANVF
Copy link
Contributor Author

ALANVF commented Jan 10, 2021

Not only is knowing the size of a map helpful sometimes, but I cannot think of any platform where the implementation would be costly whatsoever outside of AS3, which is irrelevant because it's going to be dead in 2 days anyways. As for the Makefile, there are about half a dozen open issues about Linux users have trouble installing Hashlink, so I don't see how the current setup is working as intended.

@ALANVF
Copy link
Contributor Author

ALANVF commented Jan 11, 2021

Also for reference, this is how you would get the size of a Map on other targets:

  • neko:
    • IntMap/StringMap: __dollar__hcount(map)
    • ObjectMap: __dollar__hcount(@:privateAccess map.k)
  • js:
    • IntMap/StringMap: js.Object.keys(map).length (this implementation is really bad. it should ideally be using Map instead of a js object, which is safer and has a .size() method)
    • ObjectMap: @:privateAccess map.count
  • php:
    • IntMap/StringMap: php.Global.count(map.data)
    • ObjectMap: php.Global.count(@:privateAccess map._keys)
  • python: @:privateAccess map.h.length
  • Lua: N/A, but it shouldn't matter because Lua is fast anyways (and probably even optimizes it)
    • As for technical details, different kinds of key-value pairs are stored and kept track of in different places (based on the datatype of the key), although it's all transparent on the Lua side of things so it's not exposed anywhere. You could store it separately if you really wanted to.
  • Java: @:privateAccess map.size
  • JVM:
    • StringMap: @:privateAccess map.hashMap.size()
  • C#: @:privateAccess map.size
  • Flash: N/A, but it will be dead in 2 days so it shouldn't matter anyways.
  • C++: Just expose this method

"Very costly"? It's actually the result of the devs not knowing enough about Haxe's various targets (which doesn't make sense because they implemented them?)

@skial skial mentioned this pull request Jan 11, 2021
1 task
@ncannasse
Copy link
Member

Not only is knowing the size of a map helpful sometimes

I fail to see in which case... but sure why not.

but I cannot think of any platform where the implementation would be costly whatsoever outside of AS3,
"Very costly"? It's actually the result of the devs not knowing enough about Haxe's various targets (which doesn't make sense because they implemented them?)

And you just list afterward that it's costly on JS, which is one of the main platforms, and most likely Lua as well.
We are using Object in JS because if I remember correctly it's faster than using native Map.

@ALANVF
Copy link
Contributor Author

ALANVF commented Jan 13, 2021

And you just list afterward that it's costly on JS, which is one of the main platforms, and most likely Lua as well.

I said the the current JS implementation using Objects is costly. See below for details.

As for Lua, it's not a Haxe issue, but actually a Lua issue. Iterating through all the table's entries using the pairs function and a for-loop is the only way you can get the size of a table in lua. Knowing this, it does not make sense to say that this would be costly to add, as it would be costly regardless of whether or not it's part of the core library. At least if it's implemented in the core library, it can generate a native Lua iterator instead of a Haxe iterator. If not that, you could always just keep track of the size of the table from the map object, which also fixes the problem.

We are using Object in JS because if I remember correctly it's faster than using native Map.

Quite the opposite actually. According to the MDN docs:

  • Map: Performs better in scenarios involving frequent additions and removals of key-value pairs.
  • Object: Not optimized for frequent additions and removals of key-value pairs.

@ncannasse
Copy link
Member

it's not a Haxe issue, but actually a Lua issue.

And that's exactly why we don't want to have count() on Map because of the discrependencies between platforms, some have 0 cost for the count and some requiring expensive operations.

Regarding JS: we used benchmarks across several browsers for that choice

@Gama11
Copy link
Member

Gama11 commented Jan 13, 2021

Maybe those benchmarks are out of date? Not sure if it's representative at all due to the large map size, but here a js.lib.Map performed much better: HaxeFoundation/haxe#9994

@ALANVF
Copy link
Contributor Author

ALANVF commented Feb 11, 2021

Any updates on this?

@ncannasse
Copy link
Member

Please remove the Makefile change so I can merge the primitives.
Also, errors string referencing the Haxe API should not be in HashLink sources, so please return -1 if not matched.

@ALANVF
Copy link
Contributor Author

ALANVF commented Feb 13, 2021

Done

@ncannasse ncannasse merged commit 720b509 into HaxeFoundation:master Feb 13, 2021
@ALANVF ALANVF changed the title Fix linux Makefile flags and new function for maps/regexps New function for maps/regexps Apr 22, 2021
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.

3 participants