On threads and global variables

[dwarring]

globals

LibXML has a fair number of global variables, including such things as error states, parser callbacks, and options.

Perl 5 does try to compensate, and improve reentrant behaviour, by save and restoring state, resetting options etc.

It just a matter of how hard you push it. Something's will cause problems in a single threaded environment, e.g. trying to work with multiple concurrent push or pull parsers.

threading

In the simple case LibXML doesn't support concurrent update of read/update +DOM threads across threads. There's a native lock method that's designed to help with this. This uses libXML mutexs to lock between threads. The nodes must be identical - not just in the same tree. The simplist case is to lock the document or document root element.

LibXML has been retrofitted to context switch between threads, saving and restoring state, under the control of a mutex lock. This means that some cases, tasks which aren't re-entrant may not cause contention when performed across threads.

Locking and update needs to be explored and tested more before being documented. I'd at least like to try and come up with a simple scenario where a re-entrant problem is solved by running across threads.

[vrurg]

Without knowing that LibXML is not ready for threading I actually started using it this way. So far it performs well and the only problem I'm experiencing so far is related to use of required to overcome circular dependencies. Since require as such is not thread-safe it is causing use of VMNull object sporadically.

There are a few approaches to the problem.

1. Re-structure the code to get rid of circularities. Not sure if this is possible, but it should be. For example, LibXML::Node is often used for the purpose of importing iterate-set or iterate-list subs. These can be moved into a dedicated module (say, into LibXML::Utils). As long as neither LibXML::Node::Set, nor LibXML::Node::List are using the subs this could be the best solution as they can be just use-d then by LibXML::Util.

Perhaps other cases can be resolved in a similar way.

2. Methods like xpath-class can be replaced with attributes and turned into lazy-initialized ones with AttrX::Mooish. They would then be initialized exactly once on the first read and then just used afterwards. This would result in minimal performance impact.
3. The alternative would be to use extra checks and locks withing the xpath-class method and alikes resulting in higher performance cost.

I could try implementing either approach, but would like to avoid these not appreciated by the author. :)

[dwarring]

Thanks, I haven't taken threading much further than porting 90threads.t and following libxml documentation on threading.

I went through and made a list of required classes. I did a couple of small refactors, so please update from master.

Class               Method            Required Class
-----------------   ---------------   ----------------------
LibXML::Document    implementation    LibXML
                    parser            LibXML::Parser
LibXML::Config      have-reader       LibXML::Reader
LibXML::Node        attributes        LibXML::Attr::Map
                    xpath-class       LibXML::XPath::Context
                    sub iterate-list  LibXML::Node::List
                    sub iterate-set   LibXML::Node::Set
LibXML::Node::Set   Hash              LibXML::HashMap::NodeSet

The implementation, parser and have-reader should only be used occasionally at set up. I don't think there's performance
issues anyway.

I only added attributes method to Node the DOM lets you call it on any type of Node and return undefined if its not an element. I'd considered deprecating it anyway.

Agree with treatment of xpath-class, iterate-set and iterate-list and with either solution for xpath-class.

The LibXML::Node::Set Hash method has a another require is somewhat experimental. I don't think it's widely used yet.

I think the DOM is naturally circular and a box-class or similar will always be needed. This already has locking which is hopefully doing the job.

Your approach seems reasonable and I'm happy for any assistance.

[vrurg]

Somewhat accidentally, the refactoring took me somewhat beyond just thread-safety. As I started modifying LibXML::Config a thought crossed my mind which I already had while working on my application: the config must be local to a parser instance and shared by all objects produced from that parser.

For example, max-errors is a global value. But what if an application must process both HTML and XML documents? Whereas for HTML I would sometimes turn the errors off altogether (I used to see some really bad web sites!), XML may require more restrictive approach.

I made some few steps towards the approach where is rw methods behave differently depending on wether invoked on the class or on an instance of the class. The former sets defaults. The instance then is initialized from the defaults but could be adjusted for the needs of a particular parser.

So far so good. The problem is to propagate the config object among document-bound elements. For this an element must either now its document object, where the root config is kept, or have the config object itself. Unfortunately, both approaches require and attribute – and it is not possible for CPointer representation, which is what LibXML::Element and LibXML::Node are.

Aside of this, I'm rather bad when it comes to nativecall because I never really used it. Therefore I'm not sure what the best solution to that could be. My intention is to extract the native parts of both classes into separate ones. Something like LibXML::Raw::Node and turn method raw into $.raw.

Up to my understanding, this should change very little from the user point of view as API would remain unchanged. But what if I'm missing something? An advise would be welcomed. :)

BTW, we can use IRC. I'm hanging out on both #raku and #raku-dev and usually available to pings.

[dwarring]

I used CPointer for performance reasons. Benchmarking showed nativecast to CPointer objects to be quicker than new to P6opaque. All DOM classes other than LibXML::Document are CPointer.

These could be converted to P6opaque, to allow extra attributes, but with slower object creation.

Another approach is to change the Config to a CStruct and maintain copies within C, which can then be retrived back from the raw objects. There's an xmlRef structure defined in src\xml6_gbl.c, which could be extended.

I happy to attempt this, if you want.

[vrurg]

I used CPointer for performance reasons. Benchmarking showed nativecast to CPointer objects to be quicker than new to P6opaque.

Makes full sense since there is much less to be done to build an object.

Another approach is to change the Config to a CStruct and maintain copies within C, which can then be retrived back from the raw objects. There's an xmlRef structure defined in src\xml6_gbl.c, which could be extended.

This would, probably, be the best. Though I don't see how it would maintain a single config per document principle. But this is rather due to my lack of in general understanding of the guts.

Let me try to express the problem as I see, which is no easy as I'm still trying to wrap my mind around all this. :)

1. We set default in the global config
2. A new parser instance gets a copy of it (implementation is irrelevant here)
3. A new document object somehow has access to parser's config. It could be done via keeping a reference to the parser too
4. Any sub-node of the document can access the same config too

All this would be necessary to let appendWellBalancedChunk method of LibXML::Element to work as expected by passing the configuration the element was created with into document fragment parser.

Sorry if it all sounds clumsy, but in a way I'm trying to express it all just to get better view of the problem myself. :)

In either way I'm going to rollback all the config-related changes and produce a draft PR.

[dwarring]

Thanks. I'll look at the practicality of storing config for documents over the next few week. The _private field, which is available on all nodes, including the document root is already being used to store the (poorly named) xmlRef struct, which is owned by the Raku bindings.

[dwarring]

I had a preconception when raising this ticket that libxml was sharing global state and setting between threads.
After reviewing thread saftey and threads.

Keeping one singleton global config that is definitely wrong.

Currently parsing is protected by a lock that prevents multiple threads from parsing concurrently. But hopefully that can be relaxed after fixing these issues.

[dwarring]

LibXML 0.6.16 has been released which adds the LibXML::_Configurable role to various contexts for parsing, reading, validation and xpaths, giving them a config attribute. For example:

my LibXML::Config $config .= new: :max-errors(10000);
my LibXML $parser .= new: :$config;

The node methods that were accessing global config should (If I've got them all) now take a :$config option. For example the DOM parseBalancedChunk method now takes a :$config option.

[dwarring]

Work-in-progress on the . Some of the global settings have been made thread safe.

Input callbacks are stored in a static array within libxml and aren't thread-safe. The Raku bindings currently allow per-parser input callbacks. Simplest interim solution is to deprecate allow global configuration only.

Edit: Working on a solution where a lock is held for a short duration while the input callbacks are setup at parser creation.

[dwarring]

Current solution for input-callbacks is to allow them to be configured globally, which is thread safe.

I've introduced another config setting parser-locking, which when set disables multi-threaded parsing. This must be set to enable multiple sets of per-parser input-callbacks to avert potential thread-safety issues.

I think it would be easier to fix threaded use of multiple input callbacks properly within the libxml library itself.

[dwarring]

LibXML 0.7.0 has been released which removes parser locks, except when localized sets of input callbacks are being used.

[dwarring]

Note that Perl's XML::LibXML module also has input-callback/threading issues. A bit worse because the input callbacks are always set up from scratch, rather than being set locally.

[vrurg]

I have an interesting observation to share. With my recent attempt to make require thread-safe I have also tried to replace all use of thread-blocking Lock with a new await-based Lock::Soft. This had a very confusing consequence on LibXML with t/00threads.t failing as soon as I removed the lock from LibXML::Types::resolve-package. Took me hours looking elsewhere until after tracing down to method TagExpansion I noticed that it's bypassing to xml6_gbl_[set|get]_tag_expansion. And that neither function is using any kind of handler/descriptor. This lead me to a guess that both functions are using thread-id as the handler internally.

If I'm correct then t/00threads.t works by pure accident. Since in Raku non-blocking awaiter allows ThreadPoolScheduler to re-assign a call stack to a different thread after await succeeds, and only an acquired Lock prevents this from happening (by switching to the blocking awaiter), the test currently works because there no non-blocking await is invoked between .tag-expansion = ($i + $j + 1) %% 2; and .tag-expansion == ($i + $j + 1) %% 2;. The problem did show up immediately when Lock::Soft was used internally by the core because NativeCall is actively using require, which, in turn, results in await in CompUnit code.

I'm currently reverting back to Lock anyway, so the test would not be affected. But the problem as such will remain there. Especially as long as I see more xml6_gbl_ prefixes in LibXML::Raw.

[dwarring]

I'll have a look at xml6_gbl_[set|get]_tag_expansion and the underlying calls to xmlKeepBlanksDefaultValue which I assumed to be handling the thread access.

Edit: xmlKeepBlanksDefaultValue is a macro that expands to (*(__xmlKeepBlanksDefaultValue())) which I understand to be a thread-safe and thread specific assessor. That's the reason it's invoked via binding C code rather than directly via NativeCall.

[dwarring]

I've added additional low-level subtest to t/00threads.t that bypass LibXML::Config. The first directly calls xml6_gbl_set_tag_expansion and xml6_gbl_get_tag_expansion. The second set it via the LibXML::Raw.TagExpansion proxy method.

[vrurg]

But both functions operate on xmlSaveNoEmptyTags, which is a global and cannot be thread-safe by definition. I'm rather amazed that the test is passing somehow.

[dwarring]

There's some deep magic in libxml2. xmlSaveNoEmptyTags is a macro:

The xml6_gbl.c source code is

DLLEXPORT int xml6_gbl_get_tag_expansion(void) {
    return xmlSaveNoEmptyTags;
}

DLLEXPORT void xml6_gbl_set_tag_expansion(int flag) {
    xmlSaveNoEmptyTags = flag;
}

After running through the C pre-processor and dumping the result (gcc -E flag):

# 47 "src/xml6_gbl.c"
extern int xml6_gbl_get_tag_expansion(void) {
    return (*(__xmlSaveNoEmptyTags()));
}

extern void xml6_gbl_set_tag_expansion(int flag) {
    (*(__xmlSaveNoEmptyTags())) = flag;
}

The definition of this function in the libxml library is here: https://github.com/GNOME/libxml2/blob/fe9f76ebb8127e77cbbf25d9235ceb523d3a4a92/globals.c#L1010

undef	xmlSaveNoEmptyTags
int *
__xmlSaveNoEmptyTags(void) {
    if (IS_MAIN_THREAD)
	return (&xmlSaveNoEmptyTags);
    else
	return (&xmlGetGlobalState()->xmlSaveNoEmptyTags);
}

My understanding is that the variable isn't really global, but scoped to the current thread by xmlGetGlobalState

[dwarring]

I had thought about trying to call xmlGetGlobalStae explicitly, rather than relying on this magic in xml6_glbl.c. Not sure if it it'd work better.

Edit: link https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-threads.html#xmlGetGlobalState

[vrurg]

My bad in overlooking globals.h when grepping through the sources. But now, as I went all the way down to where the thread magic (🙂) happens, I know there is a problem. libxml2 keeps its state per-thread, as reported by pthread_*. I.e. it is bound to the OS threads, as I suspected. Again, as I mentioned it earlier, if a non-blocking await would take place in between two calls to libxml2 there is some very much non-zero chance that the current call stack would be re-bound to a different OS thread by ThreadPoolScheduler. You can test this on your own by dumping $*THREAD.id in something like this:

constant COUNT = 20;
my @w;
for ^COUNT -> $i {
    @w.push: start {
        my $tid = $*THREAD.id;
        await Promise.in(.3.rand);
        say $*THREAD.id.fmt('%5d'), " ", $tid if $tid != $*THREAD.id;
    }
}
await @w;

To my view, the approach they chosen has another shortcoming of being counter-OO. A good OO approach in a multi-threaded environment assumes that an object is safe to be used within a single thread at every given moment in time (I'm not mentioning thread-safe classes which are just a step further in the safety direction). But the assumption doesn't prohibit the object to be used in another thread at another moment. Apparently, what it means is that a parser must be OK about migrating between threads, would that be Raku's or OS threads.

Sorry if I'm stating some obvious things here, I'm just trying to pull together all the details. The primary point is that there certainly has to be some kind of state attached to a parser or a configuration object which is independent of OS thread which created it. Unfortunately, it means xmlGetGlobalState is not an option exactly for the reason it binds the state to an OS thread. Instead, I think, xmlNewGlobalState() is what's needed.

The only thing which worries me is that xmlNewGlobalState uses xmlInitializeGlobalState which, in turn, uses a xmlThrDefMutex mutex where Thr suggest some kind of thread-dependency. But this worry is likely to be unfounded because otherwise it looks like a global mutex.

Unfortunately, by looking at LibXML::Raw I have a strong sensation of a big overhaul necessary to achieve the correct thread safety. Though maybe not everything is that bad and all is needed is just one additional attribute of xmlGlobalState type on LibXML::Raw. But then, unless I miss something, methods like TagExpansion would just turn into delegations to the state instance. Something like:

unit class LibXML::Raw;
...
has xmlGlobalState:D $.state;
...
method TagExpansion {
    $.state.xmlSaveNoEmptyTags
}

Maybe it would need to be lock-protected, but otherwise that's it. It would then only be a matter of passing the state into corresponding libxml2 calls. I just hope it does support this approach.

[dwarring]

Thanks, I hadn't considered that the OS thread may change during execution, which breaks the approach of setting up OS thread-specific globals ahead of time.

The LibXML code-base is taking the same approach internally as xml6_gbl.c is currently doing - relying on macros such as xmlSaveNoEmptyTags to access OS-thread specific settings. So I don't think it's going to be straight-forward.

[vrurg]

The LibXML code-base is taking the same approach internally as xml6_gbl.c is currently doing - relying on macros such as xmlSaveNoEmptyTags to access OS-thread specific settings.

This is wrong on another level too. Let's say I'm building up a tree from different sources. I.e. there is a root tree and I attach sub-trees from another sources. They may require different configuration. If no multi-threading is planned it'd be much more straightforward to use different pre-set states rather than mutate a single global one.

[dwarring]

I agree. I'm thinking it may be easier to solve in the LibXML library rather than to try to work around it in the Raku bindings. Possibly LibXML's xmlGetGlobalState() should itself be configurable or overridable to allow user defined scoping.

…

On Tue, May 31, 2022 at 2:00 AM Vadim Belman ***@***.***> wrote: The LibXML code-base is taking the same approach internally as xml6_gbl.c is currently doing - relying on macros such as xmlSaveNoEmptyTags to access OS-thread specific settings. This is wrong on another level too. Let's say I'm building up a tree from different sources. I.e. there is a root tree and I attach sub-trees from another sources. They may require different configuration. If no multi-threading is planned it'd be much more straightforward to use different pre-set states rather than mutate a single global one. — Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACC5L63YS3WVJWBWJUPND3VMTCWLANCNFSM4ISY54WA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[vrurg]

I agree. I'm thinking it may be easier to solve in the LibXML library rather than to try to work around it in the Raku bindings.

I had similar thoughts. Though I didn't look deeper into the code anyway.

[dwarring]

I made some changes with 053d759 which hopefully works better with current versions of LibXML. Config settings are now only actioned when the setup method is called. They can restored using the restore method.

All globals are now set just before they're needed and restored immediately after, e.g.

LibXML-raku/lib/LibXML/Parser/Context.rakumod

Line 116 in 053d759

my @prev = self.config.setup();

restore is called aLEAVE phaser just below this.

Hoing its safer and$*THREAD.id isn't changing in this block of protected code.

[vrurg]

Looks like an acceptable hack in a situation where there is no better solution. 😉

[vrurg]

Long time no see! :)

There is a thing to mention about the changing $*THREAD.id. There is now another dynamic which does uniquely identify a call stack: $*STACK-ID. No matter how the current thread is changed, this one can be used reliably. Though, there is always a potential problem of using two parsers within the same thread.

[vrurg]

There is one more issue about globals which I'd like to find a solution for. It is @ClassMap, box-class, ast-to-xml, and other globalish subs.

My task for today is to create a basic-level SVG parser. I would need to pull out a few bits of information out of it. Best if I could instantly create subclasses of LibXML::Element based on element name. I.e. something like SVG::Path, SVG::Group, etc. I.e. <g ... >...</g> -> SVG::Group. The first thing which comes to mind is to replace XML_NODE_ELEMENT in @ClassMap to, say, my SVG::Element class whose method new would then remap into a specific destination class.

Unfortunately, that wouldn't work for me because another parser would be working on a different kind of XML and this could happen same time in a parallel thread or earlier/later in the same one. Mangling with the mapping array is unsafe by definition.

BTW, I was already solving similar task for that other kind of XML and did it by introducing a container class which delegates to a LibXML::Node except for findnodes, which, in turn and eventually, still delegates to the node but then wraps all results into new container instances. Not the most elegant solution, not to mention both memory and processing times implications. Can and likely will use similar solution for SVG, but wish there be a better way!

So, back on track. I tried to find a solution for the task sufficiently good for a PR, but failed to do it fast. Hence just a few thoughts I came up with.

1. @ClassMap must be replaced with a method on configuration object. Best, perhaps, if box-class migrates to the configuration. This would allow to create a subclass of LibXML::Config, or just allow box-class to operate on a local copy of the mapping.
2. With the previous in mind, @ClassMap can be a default global which gets COW semantics when user wants to modify it. I.e. $parser.config.set_map(XML_NODE_ELEMENT, 'MyElement') could result in @ClassMap copied into an attribute which is then used by box-class.
3. Other globalish subs are, perhaps, good candidates to be moved into LibXML::Config too. Haven't considered this part really thorough.
4. There is a problem with non-configurable classes, where box-class and alike are used. For those a workaround of using a dynamic variable can be used if it is guaranteed that the corresponding methods are invoked by some configurable instances. In this case setting $*LIBXML-CONFIG to the configuration object would allow to ensure that they use the same mappings.
5. If the condition of the previous item cannot be fulfilled as a user might need to invoke corresponding methods in their code, a callback-like workaround can be used:

   $xml-node.context: { $non-configurable-instance.do-something };
   $parser.context: { $non-configurable-instance.do-something };

6. For backward compatibility the subs can delegate to LibXML::Configuration class methods of the same name. I.e. sub box-class(...) { LibXML::Config.box-class }. The class method would only use the default @ClassMap.

I'm certain, there are gaps in my plan. Unfortunately, that's as much as I managed for now.

[dwarring]

Still considering this as well. I'm not so fussed on preserving the global @ClassMap, if it's complicating matters. Its not ideal for reasons you've already mentioned.

[vrurg]

I've got approval for trying to solve the issue and looking into it right now, as you commented. Some important details have slipped off my mind like Node been is a CStruct. So, I don't currently see a way for a node instance to get hold of the config which was used to create it.

@ClassMap is not necessarily to be removed altogether. Here is a prototype for COW semantics I currently have in mind:

# COW clone of @ClassMap
has @!class-map;

proto method map-class(|) {*}
multi method map-class(Int:D $id, Mu:U \id-class) {
    self.protect: { @!class-map[$id] := id-class }
}

proto method box-class(|) {*}
...
multi method box-class(::?CLASS:D: Int:D $id) {
    @!class-map[$id]:exists ?? @!class-map[$id] !! resolve-package(@ClassMap[$id])
}

It is a multi because there also needs to be a Positional prototype for batch-setting the mappings.

I'm also moved box-class sub into Config and turned it into a method. For backward compatibility &LibXML::Item::box-class delegates to LibXML::Config.box-class. Was considering moving ast-to-xml as well, but stumbled upon the uselessness of it all without Node, or even better – Item, having access to the config, as I mentioned in the beginning.

Trying to wrap my brain around it all but in vain so far. Do have any suggestions, perhaps?

[dwarring]

one important details have slipped off my mind like Node been is a CStruct

Note that t/00subclass.t demonstrates sub-classing from a CPointer class (LibXML::Element) to a normal P6opaque class.

[vrurg]

That is not a problem. The problem is that it would be ideal to provide LinXML::Item with access to the config!

I have a thought on the performance, but need to do some benchmarking first.

[vrurg]

CPointer representation not only not faster on object creation, it results in worst results when it comes to object creation performance! Just to make sure I have missed nothing here is the benchmarking script:

use v6;
use nqp;

class CCPointer is repr('CPointer') { 
}
class Cnew { 
    has int32 $.n;
    method !SET-SELF(:$!n = 0) { self }
    method new(*%c) {
        nqp::create(self)!SET-SELF(|%c)
    }
}
class Cdefault {
    has Int $.n = 0;
}

constant WARM-UPS = 100000;
constant REPETITIONS = 1000000;

sub bench-type(\type, $warmup = False) {
    my $reps = $warmup ?? WARM-UPS !! REPETITIONS;

    my $dst = now;
    for ^$reps {
        my $inst = type.new();
    }
    now - $dst;
}

my @h = "default", "CPointer", "own new()";
say @h.join(", ");
for ^5 {
    my @res;
    for Cdefault, CCPointer, Cnew -> \type {
        bench-type(type, True);
        @res.push: bench-type(type);
    }
    say @res.join(", ");
}

I tried it on a couple of versions of Rakudo with the oldest one dating back to 2017.01! And it was the only one where the default constructor is the slowest:

default	CPointer	own new()
2.1390673	1.7824242	1.4175700
2.1319537	1.790073	1.4055290
2.106466	1.8008887	1.4085817
2.1193149	1.7682319	1.405775
2.1276389	1.7831767	1.40728906

But then 2019.03.1 is already totally different with this respect:

default	CPointer	own new()
0.474674	1.90318245	1.6710003
0.4589754	1.88491895	1.6675051
0.4564273	1.87175974	1.6589856
0.4594399	1.8796383	1.6559341
0.4567367	1.8664122	1.6588461

Not to mention the current master is even better:

default	CPointer	own new()
0.356861899	0.8253926	0.947033948
0.361350136	0.831525434	0.581434684
0.361931037	0.834780201	0.57964683
0.360557776	0.829500129	0.573319093
0.357812766	0.83124914	0.575271638

But since the primary point is to allow to have at least one attribute on LibXML::Item (at least, this is my point! :) ), I should have tested it with type.new(:n($_)):

default	CPointer	own new()
0.746054572	1.220703211	2.351689549
0.746012655	1.198038095	2.368736357
0.743345995	1.222338303	2.368202977
0.735399809	1.181962643	2.335892506
0.7115013	1.174389664	2.339973885

Apparently, it got slower, but the default class construction is still outperforms CPointer – event the case when its new is invoked without arguments!

My bottom line is: CPointer to be removed from where it is not really needed. This actually opens the path for making it possible to remember the parser instance used to create an object and solve many threading-related issues alongside.