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] External context creation without callbacks #2510

Conversation

akosthekiss
Copy link
Member

This change separates the concept of the external heap from the
external context, i.e., the context and the heap may be allocated
in separate memory regions by the user, and what's more, it puts
the allocation under the direct control of the user without any
need for a callback mechanism (opposed to the existing approach
based on jerry_context_alloc_t callbacks).

The here-implemented approach

  • enables the same context/heap separation in the external case
    that is already available in the global internal context
    configuration, and
  • enables statically allocated external heaps.

However, as this change gives allocation in the hands of the user,
the binding of the heap to the context can only happen when the
engine is initialised. This necessitates the change of the
arguments of jerry_init, which is a major API break (as that
function cannot be avoided in any scenario involving the engine.)

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

This change separates the concept of the external heap from the
external context, i.e., the context and the heap may be allocated
in separate memory regions by the user, and what's more, it puts
the allocation under the direct control of the user without any
need for a callback mechanism (opposed to the existing approach
based on `jerry_context_alloc_t` callbacks).

The here-implemented approach
- enables the same context/heap separation in the external case
  that is already available in the global internal context
  configuration, and
- enables statically allocated external heaps.

However, as this change gives allocation in the hands of the user,
the binding of the heap to the context can only happen when the
engine is initialised. This necessitates the change of the
arguments of `jerry_init`, which is a major API break (as that
function cannot be avoided in any scenario involving the engine.)

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]
@akosthekiss akosthekiss added ecma core Related to core ECMA functionality api Related to the public API labels Sep 4, 2018
@akosthekiss
Copy link
Member Author

This might be part of the context rework PR series, follow-up to #2501.

As this PR might induce a big churn both in the project repo and in user code base (see: all native command line tools, target applications, unit tests, and doctests had to be touched), I tag it as 'request for comments'. Although I do believe that it has several benefits (e.g., the use of a struct as its argument makes jerry_init more easily extensible in the future), others may judge that its costs outweigh them. Or that only part of its ideas should be imported in the project. Or... So, looking for (constructive) feedbacks.

To help discussions, I've prepared some examples on how external contexts could be used before/after this proposal.

Before

External context with dedicated dynamically allocated heap
(tools/build.py --external-context)

void *context_alloc (size_t size, void *cb_data_p)
{
  (void) cb_data_p;
  return malloc (size);
}

jerry_context_t *context_p = jerry_create_context (512 * 1024, context_alloc, NULL);
jerry_port_default_set_context (context_p); // default port implementation is just an example, should be platform-specific

jerry_init (JERRY_INIT_EMPTY);

jerry_cleanup ();
free (context_p);

External context with heap provided by system allocator (malloc used within the engine)
(tools/build.py --external-context --system-allocator)

void *context_alloc (size_t size, void *cb_data_p)
{
  (void) cb_data_p;
  return malloc (size);
}

jerry_context_t *context_p = jerry_create_context (0, context_alloc, NULL);
jerry_port_default_set_context (context_p);

jerry_init (JERRY_INIT_EMPTY);

jerry_cleanup ();
free (context_p);

After

External context with dedicated dynamically allocated heap
(tools/build.py --external-context)

const size_t context_size = jerry_context_size ();
jerry_context_t *context_p = malloc (context_size); // malloc is just an example, can be any platform-specific allocator
memset (context_p, 0, context_size);
jerry_port_default_set_context (context_p);

const uint32_t heap_size = 512 * 1024;
void *heap_p = malloc (heap_size);

jerry_init (JERRY_INIT_DATA (JERRY_INIT_EMPTY, heap_p, heap_size));

jerry_cleanup ();
free (context_p); // free is just an example, should be the counterpart of the allocator used above
free (heap_p);

External context with dedicated statically allocated heap
(tools/build.py --external-context)

#define HEAP_SIZE (512 * 1024)
static uint8_t heap[HEAP_SIZE];

const size_t context_size = jerry_context_size ();
jerry_context_t *context_p = malloc (context_size);
memset (context_p, 0, context_size);
jerry_port_default_set_context (context_p);

jerry_init (JERRY_INIT_DATA (JERRY_INIT_EMPTY, heap, HEAP_SIZE));

jerry_cleanup ();
free (context_p);

External context with heap provided by system allocator (malloc used within the engine)
(tools/build.py --external-context --system-allocator)

const size_t context_size = jerry_context_size ();
jerry_context_t *context_p = malloc (context_size);
memset (context_p, 0, context_size);
jerry_port_default_set_context (context_p);

jerry_init (JERRY_INIT_DEFAULT);

jerry_cleanup ();
free (context_p);

@akosthekiss
Copy link
Member Author

Note: I couldn't come up with a solution where no dynamic memory allocation was needed for jerry_context_t. As it is an opaque type and its size varies depending on how the engine was configured/built, I could retrieve its size only via a function (jerry_context_size ()). But that's not a compile time constant, so it cannot be used in a static allocation like static uint8_t context[jerry_context_size ()];.

Note 2: The above approach might also have alignment problems, although compiler extensions or C11's stdalign.h + alignas + max_align_t might solve that issue.

Still, should a solution exist for static allocation of context structs, it would be doubleplusgood to find, as dynamic memory allocation (malloc) is frowned upon (or even forbidden) in some setups.

@zherczeg
Copy link
Member

What about doing the same thing as we do with debugger: after jerry_init() a function is called to pass the heap. This way there is no need to change the jerry_init() function.

@LaszloLango
Copy link
Contributor

I like this change, but I'm a bit worrying about the impact of this patch. Changing such a basic API function might be risky. We have to be considered and cautious. I would like to here the opinion of others from the community.

@rerobika
Copy link
Member

@akosthekiss Are you planning to update this PR or has it been abandoned? I think this patch requires a big API change, so it would be good to clarify before the forthcoming 2.0 release.

@dbatyai dbatyai mentioned this pull request Sep 8, 2020
@lygstate
Copy link
Contributor

Does that means only ONE jerryscript engine can exist in a single program?

@akosthekiss
Copy link
Member Author

@lygstate No, JerryScript already supports multiple execution contexts inside a process. This PR was more about how to create those contexts.

@LaszloLango
Copy link
Contributor

@akosthekiss Is this still valid or can we close this?

@LaszloLango LaszloLango added the abandoned Abandoned by reviewers or author label Nov 13, 2024
@akosthekiss
Copy link
Member Author

@LaszloLango IMO the PR is/was valid, but nobody expressed real interest since and it got bitrotten. So, I don't think it is worth updating / spending efforts to resolve all the conflicts. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned Abandoned by reviewers or author api Related to the public API ecma core Related to core ECMA functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants