open_dict is already calling the new file sync. I am hesitant to sync write_dict_key for fear of unnecessary blocking calls. I am also confused as to why that is a public api call, seems like it should only be called by write_dict_item.
-----Original Message----- From: Dylan Baker [mailto:[email protected]] Sent: Wednesday, August 13, 2014 3:37 PM To: Atwood, Matthew S Cc: [email protected]; Chad Versace Subject: Re: [Piglit] [PATCH] programs/run.py add file sync command line option Chad might have another opinion on the matter, but I think at a mininum the public methods should be synced, so open_dict, and write_dict_key should probably also be synched On Wednesday, August 13, 2014 10:34:59 PM Atwood, Matthew S wrote: > I specifically avoided calling it in helper functions to ensure that we > weren't unnecessarily syncing, we don't particularly care about a half done > test case being written, its either all or nothing in terms or whether or not > we care I think. > > -----Original Message----- > From: Dylan Baker [mailto:[email protected]] > Sent: Wednesday, August 13, 2014 3:24 PM > To: [email protected] > Cc: Atwood, Matthew S > Subject: Re: [Piglit] [PATCH] programs/run.py add file sync command > line option > > On Monday, January 02, 2012 05:09:08 PM Matthew Atwood wrote: > > From: Matt Atwood <[email protected]> > > > > Currently while running igt tests a kernel panic causes the results > > json file to lose all data. This patch adds a command line option > > (-s, > > --sync) that syncs the file descriptor to disk after every test > > allowing data to persist through hard crashes. > > --- > > framework/core.py | 3 ++- > > framework/programs/run.py | 14 ++++++++++---- > > framework/results.py | 12 +++++++++++- > > 3 files changed, 23 insertions(+), 6 deletions(-) > > > > diff --git a/framework/core.py b/framework/core.py index > > d3922a9..8c4ff57 100644 > > --- a/framework/core.py > > +++ b/framework/core.py > > @@ -100,7 +100,7 @@ class Options(object): > > """ > > def __init__(self, concurrent=True, execute=True, include_filter=None, > > exclude_filter=None, valgrind=False, dmesg=False, > > - verbose=False): > > + verbose=False, sync=False): > > self.concurrent = concurrent > > self.execute = execute > > self.filter = [re.compile(x) for x in include_filter or []] > > @@ -109,6 +109,7 @@ class Options(object): > > self.valgrind = valgrind > > self.dmesg = dmesg > > self.verbose = verbose > > + self.sync = sync > > # env is used to set some base environment variables that are not > > going > > # to change across runs, without sending them to os.environ which > > is > > # fickle as easy to break > > diff --git a/framework/programs/run.py b/framework/programs/run.py > > index eb67d7f..468bff4 100644 > > --- a/framework/programs/run.py > > +++ b/framework/programs/run.py > > @@ -97,6 +97,9 @@ def run(input_): > > type=path.realpath, > > metavar="<Results Path>", > > help="Path to results folder") > > + parser.add_argument("-s", "--sync", > > + action="store_true", > > + help="Sync results to disk after every > > + test") > > args = parser.parse_args(input_) > > > > # Disable Windows error message boxes for this and all child processes. > > @@ -132,7 +135,8 @@ def run(input_): > > execute=args.execute, > > valgrind=args.valgrind, > > dmesg=args.dmesg, > > - verbose=args.verbose) > > + verbose=args.verbose, > > + sync=args.sync) > > > > # Set the platform to pass to waffle > > if args.platform: > > @@ -154,7 +158,7 @@ def run(input_): > > # Begin json. > > result_filepath = path.join(args.results_path, 'results.json') > > result_file = open(result_filepath, 'w') > > - json_writer = framework.results.JSONWriter(result_file) > > + json_writer = framework.results.JSONWriter(result_file, > > + opts.sync) > > > > # Create a dictionary to pass to initialize json, it needs the > > contents of > > # the env dictionary and profile and platform information @@ > > -211,7 +215,8 @@ def resume(input_): > > execute=results.options['execute'], > > valgrind=results.options['valgrind'], > > dmesg=results.options['dmesg'], > > - verbose=results.options['verbose']) > > + verbose=results.options['verbose'], > > + sync=results.options['sync']) > > > > core.get_config(args.config_file) > > > > @@ -219,7 +224,8 @@ def resume(input_): > > opts.env['PIGLIT_PLATFORM'] = results.options['platform'] > > > > results_path = path.join(args.results_path, 'results.json') > > - json_writer = framework.results.JSONWriter(open(results_path, 'w+')) > > + json_writer = framework.results.JSONWriter(open(results_path, 'w+'), > > + opts.sync) > > json_writer.initialize_json(results.options, results.name, > > core.collect_system_info()) > > > > diff --git a/framework/results.py b/framework/results.py index > > 4b5fb30..d39f63b 100644 > > --- a/framework/results.py > > +++ b/framework/results.py > > @@ -102,8 +102,9 @@ class JSONWriter(object): > > > > INDENT = 4 > > > > - def __init__(self, f): > > + def __init__(self, f, fsync): > > self.file = f > > + self.fsync = fsync > > self.__indent_level = 0 > > self.__inhibit_next_indent = False > > self.__encoder = json.JSONEncoder(indent=self.INDENT, > > @@ -175,6 +176,11 @@ class JSONWriter(object): > > self.file.close() > > > > @synchronized_self > > + def __file_sync(self): > > + if self.fsync: > > + os.fsync(self.file) > > + > > + @synchronized_self > > def __write_indent(self): > > if self.__inhibit_next_indent: > > self.__inhibit_next_indent = False @@ -201,6 +207,7 @@ > > class JSONWriter(object): > > self.__indent_level += 1 > > self.__is_collection_empty.append(True) > > self._open_containers.append('dict') > > + self.__file_sync() > > > > @synchronized_self > > def close_dict(self): > > @@ -212,6 +219,7 @@ class JSONWriter(object): > > self.file.write('}') > > assert self._open_containers[-1] == 'dict' > > self._open_containers.pop() > > + self.__file_sync() > > > > @synchronized_self > > def write_dict_item(self, key, value): > > @@ -221,6 +229,8 @@ class JSONWriter(object): > > # Write value. > > self.__write(value) > > > > + self.__file_sync() > > + > > @synchronized_self > > def write_dict_key(self, key): > > # Write comma if this is not the initial item in the dict. > > I don't think that you've used your new method everywhere you want to use it, > I think that you need to call it at the end of each method except __init__. > > I think a decorator would be an elegant way to solve this, but it would be a > rather complex decorator (or, actually probably a pair of decorators), and it > would probably mean working on the synchronized_self decorator as well, since > I strongly suspect there would be a bad interaction there. > > > -- > > 1.8.3.2 > > > > _______________________________________________ > > Piglit mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/piglit _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
