Skip to content
This repository has been archived by the owner on Mar 26, 2024. It is now read-only.

Restructure build process to simplify the wasm memory fragmentation #62

Merged
merged 41 commits into from
Apr 12, 2020

Conversation

TrueDoctor
Copy link
Member

@TrueDoctor TrueDoctor commented Apr 11, 2020

Todo:

  • js parsing
  • fix bodged find function
  • general cleanup
  • use texture library instead of deprecated shared heap

@TrueDoctor TrueDoctor requested review from Ma27 and nat-rix April 11, 2020 03:14
@TrueDoctor TrueDoctor requested review from nat-rix and konsumlamm and removed request for nat-rix and Ma27 April 12, 2020 04:33
@TrueDoctor TrueDoctor force-pushed the scheduling branch 2 times, most recently from 3600d04 to 457dc0c Compare April 12, 2020 14:26
client/Makefile.toml Outdated Show resolved Hide resolved
rask-engine/src/resources/library.rs Outdated Show resolved Hide resolved
client/build.rs Outdated Show resolved Hide resolved
client/build.rs Outdated Show resolved Hide resolved
client/Makefile.toml Show resolved Hide resolved
rask-engine/src/resources/mod.rs Outdated Show resolved Hide resolved
rask-engine/src/resources/library.rs Outdated Show resolved Hide resolved
}

fn index_check(&self, id: usize) -> Result<(), EngineError> {
if id > self.0.len() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of of using a return-statement, use

if blah {
    Err(foo)
} else {
    Ok(bar)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it does not make the cod any faster, shorter or more readable

rask-engine/src/resources/library.rs Outdated Show resolved Hide resolved
impl GetStore<$type> for ResourceTable {
unsafe fn get(&'static self, id: usize) -> Result<&'static $type, EngineError> {
self.index_check(id)?;
match &self.0[id] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using Index, you could use get (and get_mut in store) for indexing (self.0.get(id)), which returns an Option. That would be much cleaner than the index_check-function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree if the requested resource is out of bounds, an error should be raised, because it is unexpected behaviour. It is essentially a fancy panic.
And I don't like the approach of get_mut(), because with the current implementation we do all the wrapping in the library which would then needed to be done by the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panic? I thought a panic could not occur in your function. I don't get your point. You could do something like

match self.0.get(id).ok_or(myerrorblahblah)? {
}

This does not make any difference for the user

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also relevant, whether the error occured because of an out of bounds, or because of a resource type mismatch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, but that does not change anything on my point. See, you could do the following:

unsafe fn get(&'a self, id: usize) -> Result<&'a $type, EngineError> {
    match self.0.get(id) {
        Some(Resource::$enum_type(value)) => Ok(&value),
        Some(_) => Err("Wrong resource type".into()),
        None => Err("Invalid resource id")
    }
}

@TrueDoctor TrueDoctor force-pushed the scheduling branch 2 times, most recently from 791b113 to 457dc0c Compare April 12, 2020 14:34
@nat-rix nat-rix self-requested a review April 12, 2020 14:34
rask-engine/src/boxes.rs Show resolved Hide resolved
}

fn index_check(&self, id: usize) -> Result<(), EngineError> {
if id > self.0.len() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

impl GetStore<$type> for ResourceTable {
unsafe fn get(&'static self, id: usize) -> Result<&'static $type, EngineError> {
self.index_check(id)?;
match &self.0[id] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panic? I thought a panic could not occur in your function. I don't get your point. You could do something like

match self.0.get(id).ok_or(myerrorblahblah)? {
}

This does not make any difference for the user

@TrueDoctor TrueDoctor force-pushed the rework-memory branch 2 times, most recently from 632b3ff to 80ec91c Compare April 12, 2020 19:26
@TrueDoctor TrueDoctor merged commit a2264af into scheduling Apr 12, 2020
@TrueDoctor TrueDoctor deleted the rework-memory branch April 12, 2020 19:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants