Document and implement doubly-linked implementation of prop (see #1040) #1059

blefloch · 2022-02-28T04:52:51Z

There remains \prop_show:N and log, which means that we cannot really
test the code seriously yet. However, it is already exercised as
part of the implementation of \prop_concat:NNN and the from_keyval
functions.

josephwright · 2023-10-14T14:59:15Z

Any news on this: I think 'large' props would be a great addition (cf. work on tagging for example).

car222222 · 2023-10-14T15:16:51Z

What about "props as hash-table" (since this seems to be in use nearly in the kernel by now)??

blefloch · 2024-01-15T12:57:43Z

I decided to rebase locally on the main branch then force-pushed rather than having a pretty painful merge. I also squashed the commits into a single one because there was no natural way to split up the code changes. For now the "added" date is today 2024-01-15 to make l3doc happy. At long last, this pull request is ready to be reviewed and merged, I think.

Skillmon · 2024-01-16T17:48:28Z

l3kernel/l3prop.dtx

+%     \cs[no-index]{prop_map_\ldots{}}.
+%
+%   \item
+%     The \enquote{linked} storage method is meant for property lists with a


Suggested change

% The \enquote{linked} storage method is meant for property lists with a

% The \enquote{linked} storage method is meant for property lists with

josephwright

Looks good to me at the code level - lets see what everyone thinks interface-wise.

FrankMittelbach

Looks good to me, but I'm not through reviewing. Any, here are some first comments on the interface section

FrankMittelbach · 2024-01-16T21:38:55Z

l3kernel/l3prop.dtx

+% using \cs{prop_new:N} (\enquote{flat} storage) or \cs{prop_new_large:N}
+% (\enquote{linked} storage). Once a property list is declared with
+% \cs{prop_new:N} or \cs{prop_new_large:N}, the type of internal data storage
+% can no longer be changed. All other \pkg{l3prop} functions transparently


While the current implementation does not over a conversion from flat to linked it should be technically simple to provide, and we should consider to offer it. Use case: property lists that are define as flat in the kernel or in a package but that in certain circumstances, e.g., when a special package is loaded, grow a lot compared to the default case. In that case it would be nice if there is a mechanism to convert from one form to another even if the original declaration happened elsewhere and the list has already entries.

More or less syntactic sugar given that the prop_set_eq:NN will basically do the work, but I think it deserves a defined name.

Easy to provide but (1) what name? (2) for local props it's probably annoying if people try to change the storage type within a group, I'll have to see how much bookkeeping is needed.

I would agree that changing this “type” only locally does not sound like a good idea.

Name: \prop_make_large:N

Note that \prop_set_eq:NN will do the conversion if needed (both ways).
So it just needs a small extension to cope with the name staying the same??

\prop_make_...:N is a good name. I also thought of \prop_renew_...:N but it might suggest that the process would give an empty prop like \prop_new_...:N. Regardless of name we need both directions I suppose.

There is a bit more work than for \prop_set_eq:NN because some auxiliary structures have to be added / deleted.

FrankMittelbach · 2024-01-16T21:51:27Z

l3kernel/l3prop.dtx

+%
+% \begin{function}[added = 2024-01-15]{\prop_new_large:N, \prop_new_large:c}
+%   \begin{syntax}
+%     \cs{prop_new_large:N} \meta{property list}


One can say that "linked" is an implementation detail and "large" is the intended use, but initially linked lists might always stay small and flat ones might grew very large. And given that "linked" also shows up in the documentation and in error messages (I think) I wonder if it would be better to call this \prop_new_linked:N and perhaps even offer \prop_new_flat:N

You correctly pinpointed my reason to prefer "large" in function names (and my inconsistency in using a different word in error messages), which is that linked is an implementation detail. In fact, the new props are both doubly-linked and implemented using the hash table, and the latter point is an important part of why they end up faster for accessing entries. Is "linked" really a good word for that? "hashed"?

I agree that names that refer to the implementation are not good (since there may be further changes to this in another decade or so).

I also understand @FrankMittelbach ‘s problem with labelling them as “large”.

Needs further thought!

Further: in some of the documentation, it is probably best to use both names.

We need a name that does not imply any “ordering” on the type of implementation, and says nothing about the expected contents, but is also independent of any details of the implementation.

FrankMittelbach · 2024-01-16T21:54:53Z

l3kernel/l3prop.dtx

+%     \prop_gclear_new_large:N, \prop_gclear_new_large:c
+%   }
+%   \begin{syntax}
+%     \cs{prop_clear_new_large:N} \meta{property list}


Here \prop_clear_new_linked:N reads a tiny bit better to me. Pity we save an "or" in the name

FrankMittelbach · 2024-01-16T22:00:24Z

l3kernel/l3prop.dtx

-%   \pkg{l3keys}), each key here \emph{must} be followed with an \texttt{=}
-%   sign.
+% \begin{function}[added = 2024-01-15]
+%   {\prop_const_large_from_keyval:Nn, \prop_const_large_from_keyval:cn}


again "linked" feels more natural to me

car222222 · 2024-01-17T04:29:20Z

The diff here is "too big" for my github app! It refuses to load it.

car222222 · 2024-01-17T06:13:44Z

l3kernel/l3prop.dtx

 % \end{function}
 %
 % \begin{function}[added = 2014-08-12, updated = 2021-04-29]{\prop_log:N, \prop_log:c}
 %   \begin{syntax}
 %     \cs{prop_log:N} \meta{property list}
 %   \end{syntax}
-%   Writes the entries in the \meta{property list} in the log file.
+%   Writes the entries in the \meta{property list} in the log file,
+%   and specifies its storage type.
 % \end{function}
 %
 % \section{Scratch property lists}


It is probably sensible to add a note here that the are no scratch “large” pls.

car222222 · 2024-01-17T06:18:37Z

But I can get the diff using Chrome.

But response is achingly slow: not sure if this is due to the size of the diff or just my connection?

So I have now added a few responses and comments on the documentation. Not yet tackled the code.

FrankMittelbach · 2024-01-17T08:55:43Z

A suggested stress test for the implementation:

force all property lists to be"large" in the test suite by making \prop_new:N equal to \prop_new_large:N in the kernel and then run the 2e test suite. It would be very interesting to see what that would do to the time it takes to run the tests in all directories (compared to the time it currently needs).

blefloch · 2024-01-17T10:40:33Z

Indeed Frank that would be a good test, but I'll need some time to set it up (pretty busy work week). I'm worried because the order of prop items is probably different with the new prop implementation than the old one, which may show up in various test logs.

car222222 · 2024-01-17T11:33:23Z

In the explanation of the "calculation" of the prefix, add something like this:

The aim here is to make this string as short as we can, given the range of distinct characters available.

(Maybe also explain why it should be as short as possible, possibly by reference to earlier where the reason is fully explained?)

u-fischer · 2024-01-17T11:47:19Z

l3kernel/l3prop.dtx

+%
+%   \item
+%     The \enquote{linked} storage method is meant for property lists with a
+%     large numbers of entries.  It has more memory overhead, but is


Imho "memory overhead" should be clarified. Half of the user will probably think that that means computer memory.

car222222 · 2024-01-17T12:46:45Z

At a later stage, the code for the production of short, unique strings should probably be moved to somewhere that makes it more widely available.

blefloch · 2024-02-11T23:38:17Z

I think I addressed all of the comments. I also found a few bugs in my code, and minor ones elsewhere (see recent issues).

Most importantly, I renamed the functions as Frank suggested, \prop_new_linked:N etc., and I had to spend quite some time fighting with the scope checking for l3debug. From my point of view this could be merged.

The small merge conflict with main is trivial to fix: keep my additional \__prop_chk:w and remove the line break. The failing tests seem related to Joseph's l3build changes. Rebasing on main should fix them I suppose.

josephwright · 2024-02-11T23:39:50Z

Looks good to me: lets see what everyone else thinks!

blefloch · 2024-02-11T23:48:39Z

Ah, one check that Frank suggested was to run the LaTeX2e suite with an additional line making all props linked. I didn't figure out how to do that, but in case someone wants to try, one has to add \cs_gset_eq:NN \prop_new:N \prop_new_linked:N after the definition of \prop_new_linked:N in l3prop.dtx, and to comment out \str_if_eq:nnF {#3} { flat } { #3~ } in l3msg.dtx. This makes a failing m3prop007 file because of some missing linked words, but it will minimize the number of test log changes on the LaTeX2e side.

blefloch · 2024-02-12T10:56:52Z

I managed to use the new linked prop in the latex2e test suite, which uncovers a problem with treatment of hashes. I'm investigating.

I also forgot to add the suggested \prop_make_linked:N and \prop_make_flat:N functions (and global versions \prop_gmake_linked:N and \prop_gmake_flat:N I suppose).

josephwright · 2024-02-12T12:41:17Z

Presumably we can apply the 'make flat' function in 2e rollback if available: either the prop is flat and 'make flat' is unvailable, or the 'make flat' function is available and will work.

blefloch · 2024-02-12T12:51:57Z

Yes. Anyways, props are flat by default, so this branch doesn't change anything in LaTeX2e. I just wanted to test whether the LaTeX2e test suite runs correctly with props being linked props. Overall it slows down the test suite slightly there, presumably because there are not that many very long props.

Despite the failing tests, I think the branch can be merged/rebased into main. Last I checked (before updating l3build with the latest normalizations), the LaTeX3 tests succeeded.

car222222 · 2024-02-12T13:39:55Z

I would think that there are no "very long props" there (depending on where "very" starts!), or some such have been explicitly and purposefully added.

The hard part was surprisingly to implement \prop_show:N and log, which have to tediously check every single detail of the internal structure of the linked property list, in a way that should be fully robust to a broken data structure. The order in which functions are defined has been modified, making the diff somewhat ugly, sorry. The new test is basically a concatenation of all the other tests, but with linked property lists instead of flat ones.

This helps get the same order of items in both implementations.

For linked props this would leave some material in the input stream. Removing \begin{document} ensures that left-over material leads to a Missing \begin{document} error.

Since some l3prop functions such as \prop_put_from_keyval:Nn use others repeatedly, one has to make it so that none of the underlying code runs any debug check (by using lower-level \cs_set_nopar:Npe etc), and add a lot of prop functions to the list of patched commands in l3debug.

blefloch added expl3 enhancement New feature or request labels Feb 28, 2022

josephwright removed the expl3 label May 17, 2023

blefloch force-pushed the gh1040-prop-large branch from ef0d119 to e9f327c Compare January 15, 2024 12:55

blefloch marked this pull request as ready for review January 15, 2024 12:57

blefloch requested a review from josephwright January 15, 2024 12:58

blefloch added the optimization Optimizations and small tweaks label Jan 15, 2024

blefloch requested a review from FrankMittelbach January 15, 2024 21:16

josephwright requested review from car222222 and davidcarlisle January 15, 2024 21:30

Skillmon reviewed Jan 16, 2024

View reviewed changes

josephwright approved these changes Jan 16, 2024

View reviewed changes

FrankMittelbach reviewed Jan 16, 2024

View reviewed changes

car222222 reviewed Jan 17, 2024

View reviewed changes

u-fischer reviewed Jan 17, 2024

View reviewed changes

blefloch mentioned this pull request Feb 11, 2024

Global coffin inconsistent local/global assignment #1443

Closed

blefloch and others added 12 commits February 13, 2024 13:49

Simplify prop_concat to run the same code for flat and linked props

8fd782a

This helps get the same order of items in both implementations.

Make a test less sensitive to implementation details

39c1d8e

Fix a completely wrong \prop_map_inline:nn and test it

011ec2c

For linked props this would leave some material in the input stream. Removing \begin{document} ensures that left-over material leads to a Missing \begin{document} error.

l3prop documentation changes suggested by Ulrike and Chris

e3d981a

In l3prop names, change _large to _linked

ca2f46c

Fix \prop_set_eq:NN for twice the same linked prop

a1ef1e0

Documentation improvements in l3prop

6cff90a

Implement \prop_make_flat:N / linked to change storage type

7934baa

Add CHANGELOG entry, update dates

2b7ef03

Update for l3build wrapping change

ddd32a4

josephwright force-pushed the gh1040-prop-large branch from cb90d95 to ddd32a4 Compare February 13, 2024 14:02

josephwright merged commit 3604e82 into main Feb 13, 2024
6 checks passed

josephwright deleted the gh1040-prop-large branch February 13, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document and implement doubly-linked implementation of prop (see #1040) #1059

Document and implement doubly-linked implementation of prop (see #1040) #1059

blefloch commented Feb 28, 2022

josephwright commented Oct 14, 2023

car222222 commented Oct 14, 2023

blefloch commented Jan 15, 2024

Skillmon Jan 16, 2024

josephwright left a comment

FrankMittelbach left a comment

FrankMittelbach Jan 16, 2024

blefloch Jan 17, 2024

car222222 Jan 17, 2024

car222222 Jan 17, 2024

blefloch Jan 17, 2024

FrankMittelbach Jan 16, 2024

blefloch Jan 17, 2024

car222222 Jan 17, 2024

car222222 Jan 17, 2024

car222222 Jan 17, 2024

FrankMittelbach Jan 16, 2024

FrankMittelbach Jan 16, 2024

car222222 commented Jan 17, 2024

car222222 Jan 17, 2024

car222222 commented Jan 17, 2024

FrankMittelbach commented Jan 17, 2024

blefloch commented Jan 17, 2024

car222222 commented Jan 17, 2024 •

edited

Loading

u-fischer Jan 17, 2024

car222222 commented Jan 17, 2024

blefloch commented Feb 11, 2024

josephwright commented Feb 11, 2024

blefloch commented Feb 11, 2024

blefloch commented Feb 12, 2024

josephwright commented Feb 12, 2024

blefloch commented Feb 12, 2024

car222222 commented Feb 12, 2024

	% The \enquote{linked} storage method is meant for property lists with a
	% The \enquote{linked} storage method is meant for property lists with

Document and implement doubly-linked implementation of prop (see #1040) #1059

Document and implement doubly-linked implementation of prop (see #1040) #1059

Conversation

blefloch commented Feb 28, 2022

josephwright commented Oct 14, 2023

car222222 commented Oct 14, 2023

blefloch commented Jan 15, 2024

Choose a reason for hiding this comment

josephwright left a comment

Choose a reason for hiding this comment

FrankMittelbach left a 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

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

car222222 commented Jan 17, 2024

Choose a reason for hiding this comment

car222222 commented Jan 17, 2024

FrankMittelbach commented Jan 17, 2024

blefloch commented Jan 17, 2024

car222222 commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

car222222 commented Jan 17, 2024

blefloch commented Feb 11, 2024

josephwright commented Feb 11, 2024

blefloch commented Feb 11, 2024

blefloch commented Feb 12, 2024

josephwright commented Feb 12, 2024

blefloch commented Feb 12, 2024

car222222 commented Feb 12, 2024

car222222 commented Jan 17, 2024 •

edited

Loading